feat: Replace AcademicYear with automatic course scheduling system - #9
Draft
SpiderQubit wants to merge 10 commits into
Draft
feat: Replace AcademicYear with automatic course scheduling system#9SpiderQubit wants to merge 10 commits into
SpiderQubit wants to merge 10 commits into
Conversation
Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/a6e01129-76ef-4f7e-9b95-aafef4f01b75 Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/a6e01129-76ef-4f7e-9b95-aafef4f01b75 Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
…casion Remove the AcademicYear model entirely. CourseOccasion.year is now a plain IntegerField (matching PrivateCourse.year). Display titles like "20/21" are computed by academic_year_title(). All year dropdowns in forms now populate dynamically (current year ± 10) with no database rows needed. Files changed: models.py, forms.py, filters.py, views.py, tables.py, admin.py, functions.py, 5 templates, 3 management commands. Deleted: ensure_academic_years management command. Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/b816d356-cc25-4262-a918-9016ea81368b Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
Remove ensure_academic_years references. Add data migration steps for copying FK year values. Update clean install section. Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/b816d356-cc25-4262-a918-9016ea81368b Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
…ean_year validation Extract YEAR_RANGE_OFFSET = 10 to models.py and use it consistently in forms.py, filters.py, and views.py instead of magic numbers. Add ValueError handling to CourseOccasionForm.clean_year(). Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/b816d356-cc25-4262-a918-9016ea81368b Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
Agent-Logs-Url: https://github.com/it-amanuens/rodatraden/sessions/29702023-4425-43ed-9218-823651cb84d1 Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
… enhance year choice logic in filters and forms
…CourseOccasion fields
Collaborator
Author
|
This is a continuation of #3 (feature/summer-courses), which was closed when the branch was deleted. This branch ( |
SpiderQubit
marked this pull request as ready for review
April 18, 2026 14:21
…s and adjust template for better display
SpiderQubit
added
enhancement
📝 WalkthroughWalkthroughThis pull request removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Collaborator
Author
|
@copilot resolve the merge conflicts in this pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rodatraden/rodatraden_modules/functions.py (1)
42-49:⚠️ Potential issue | 🔴 CriticalCatch specific exceptions instead of all exceptions in import mapping.
At line 48, bare
except:silently hides unexpected failures (DB errors, query logic errors, etc.) and drops course occasions without visibility. Only the expected lookup cases should be skipped.Proposed fix
try: new_course_occasion = CourseOccasion.objects.get( course = course_occasion.course, year = new_year, start = course_occasion.start ) new_course_occasions.append(new_course_occasion) - except: - pass + except CourseOccasion.DoesNotExist: + continue + except CourseOccasion.MultipleObjectsReturned: + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/rodatraden_modules/functions.py` around lines 42 - 49, Replace the bare except that swallows all errors when resolving CourseOccasion with explicit exception handling: catch CourseOccasion.DoesNotExist to skip missing mappings and optionally catch CourseOccasion.MultipleObjectsReturned to handle duplicates (e.g., log or choose one) and re-raise or log any other unexpected exceptions instead of silently passing; update the block around the CourseOccasion.objects.get call that populates new_course_occasions (referencing course_occasion and new_year) to use these specific exception handlers so only expected lookup misses are ignored.rodatraden/forms.py (2)
187-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude existing blacklisted years in the edit choices.
year_choicesonly preservesstart_yearandend_year. If an existing segment has a blacklisted year outside the current ±10 window, Line 214 seeds it intoinitial, but Line 204 never offers that value as a selectable choice, so a no-op edit can silently drop the exclusion on save.Suggested fix
- include_years = [self.instance.start_year, self.instance.end_year] if self.instance.pk else None + include_years = [] + if self.instance.pk: + include_years.extend([ + self.instance.start_year, + self.instance.end_year, + *(self.instance.blacklisted_years or []), + ]) year_choices = [ (str(y), label) - for y, label in _dynamic_year_choices(include_years=include_years) + for y, label in _dynamic_year_choices(include_years=include_years) ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/forms.py` around lines 187 - 216, The year choices generation (year_choices) currently only includes self.instance.start_year and end_year, so existing blacklisted years outside that range can be lost; update the include_years value passed to _dynamic_year_choices to also include any years from self.instance.blacklisted_years when self.instance.pk is truthy (merge start_year, end_year and self.instance.blacklisted_years, deduplicate and sort), then build year_choices from that combined list so self.fields['blacklisted_years'].choices and the initial population (self.initial['blacklisted_years']) can retain and show those years as selectable options.
243-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve copied
yearandstartwhen cloning a course occasion.Line 260 sets the copied
starton the original model-backed field, but Line 270 replaces that field with a freshStartWeekField, so the cloned form falls back to the default start. The copied year can also disappear from the dropdown when it sits outside the dynamic window because the choices only includeself.instance.year.Suggested fix
def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + courseocc_cpy = None + if 'courseocc' in self.request.GET: + courseocc_cpy = CourseOccasion.objects.get( + slug=self.request.GET['courseocc'] + ) # Year is now a plain IntegerField — present as a dropdown with # dynamically computed year choices (current year ± 10). year_choices = _dynamic_year_choices( - include_years=[self.instance.year] if self.instance.pk else None + include_years=[ + y for y in [ + self.instance.year if self.instance.pk else None, + courseocc_cpy.year if courseocc_cpy else None, + ] if y is not None + ] ) self.fields['year'] = forms.ChoiceField( choices=year_choices, initial=datetime.datetime.now().year, label='År', ) + self.fields['start'] = StartWeekField(label='Läsperiod') # If a courseoccasion is copied - if 'courseocc' in self.request.GET: - courseocc_cpy = CourseOccasion.objects.get(slug=self.request.GET['courseocc']) + if courseocc_cpy: # This hard-coding might be avoidable. Not sure... self.fields['course'].initial = courseocc_cpy.course self.fields['year'].initial = courseocc_cpy.year - self.fields['start'].initial = courseocc_cpy.start + self.initial['start'] = courseocc_cpy.start self.fields['weeks'].initial = courseocc_cpy.weeks self.fields['note'].initial = courseocc_cpy.note self.fields['contact_name'].initial = courseocc_cpy.contact_name self.fields['contact_email'].initial = courseocc_cpy.contact_email self.fields['official'].initial = courseocc_cpy.official self.fields['course'].widget.attrs['class'] = \ 'course-list-filter-courseocc-create' - - self.fields['start'] = StartWeekField(label='Läsperiod')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/forms.py` around lines 243 - 270, The cloned courseoccasion's year and start get lost because you build year_choices with include_years based only on self.instance.year (which is empty for a clone) and you replace the model-backed start field with StartWeekField after setting its initial; fix by ensuring the copied year is included in the dynamic choices and the copied start is applied to the final StartWeekField: when computing year_choices, pass include_years=[self.instance.year] if editing else include_years=[courseocc_cpy.year] when 'courseocc' present (or merge both), and either create/assign self.fields['start'] = StartWeekField(...) before you set courseocc_cpy.start on self.fields['start'].initial or set self.fields['start'].initial after instantiating StartWeekField so the copied start value persists. Ensure you still set self.fields['year'].initial = courseocc_cpy.year after adjusting choices.rodatraden/views.py (1)
103-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the scheduling tool by
can_manage_scheduling, not only staff.The PR introduces
rodatraden.can_manage_schedulingas the feature gate, but/verktyg/still rejects non-staff users even if they have that permission. That makes the new permission ineffective for the global scheduling tool.Suggested fix
- # Only staff can access this site - if not request.user.is_staff: + # Scheduling tools are permission-gated. + if not request.user.has_perm('rodatraden.can_manage_scheduling'): return redirect(reverse('index'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/views.py` around lines 103 - 105, The view currently gates access by checking request.user.is_staff and redirects with return redirect(reverse('index')); update the access check to also allow users with the new permission by testing request.user.has_perm('rodatraden.can_manage_scheduling') (i.e., replace or augment the is_staff check with a logical OR so users who have can_manage_scheduling are permitted); keep the existing redirect(reverse('index')) behavior for users who lack both the staff flag and the can_manage_scheduling permission.
🧹 Nitpick comments (1)
rodatraden/management/commands/generate_course_occasions.py (1)
77-84: 💤 Low valuePeriod calculation duplicated across commands.
The
period_number = segment.start // weeks_in_period + 1formula is duplicated here and ininfer_course_scheduling.py:155. The same logic exists inCourseOccasion.period_label()(models.py:550-552). Consider extracting this to a shared utility function or using the model's existing property to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/management/commands/generate_course_occasions.py` around lines 77 - 84, The period-number calculation (period_number = segment.start // weeks_in_period + 1) is duplicated; replace this inline logic by calling a shared helper or the model property to avoid duplication—either extract a utility like compute_period_number(start, weeks_in_period) and use it here and in infer_course_scheduling.py, or use the existing CourseOccasion.period_label()/period-related property from models.py (and reference weeks_in_period as the canonical constant) so both commands derive the period label from the same single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MIGRATION_GUIDE.md`:
- Around line 45-49: The migration guide currently tells operators to run the
hard-coded command "git pull origin main" which is wrong for this PR's target
branch; update the command in MIGRATION_GUIDE.md by replacing "git pull origin
main" with the correct target branch for this PR (e.g., "git pull origin
master") or, better, use a placeholder like "git pull origin <target-branch>" so
operators pull the branch named in the PR; ensure the literal "git pull origin
main" is removed or replaced accordingly.
In `@rodatraden/filters.py`:
- Around line 81-88: The filter is currently matching exact start weeks and uses
wrong LP values; change the ChoiceFilter named time_period to emit LP base
offsets as (i-1)*10 (so LP1 -> 0, LP5 -> 40) and switch it to a method filter
(add method='filter_time_period') instead of field_name='start' so you can
return occasions whose start falls inside the LP window (start between base and
base+9). Implement a FilterSet method filter_time_period(self, queryset, name,
value) that parses the chosen base offset, computes base=int(value) and filters
queryset.filter(start__gte=base, start__lte=base+9) (or start__lt=base+10) and
leave the human label choices as before.
In `@rodatraden/views.py`:
- Around line 115-124: The current loop in CourseOccasion copying uses an
existence check that only filters by year and course, causing multiple distinct
source occasions to collapse into one; update the check in the block that
references CourseOccasion and courseocc so it compares all fields that make an
occasion distinct (for example include start_date/start_time, end_time,
location, and any other unique keys) such that you do:
CourseOccasion.objects.filter(year=to_year, course=courseocc.course,
start_date=courseocc.start_date, start_time=courseocc.start_time,
location=courseocc.location) (or the equivalent set of unique fields for your
model) before skipping creation, ensuring each distinct source occasion is
copied.
- Around line 156-169: The loop building segment_years treats seg.end_year None
as seg.start_year only, skipping historical years for open-ended segments;
compute current = datetime.date.today().year before the loop and when iterating
segments_all set end = seg.end_year if present else current - 1 (and ensure end
= max(end, seg.start_year)), then update segment_years with
range(seg.start_year, end + 1) so all_years includes past years for open-ended
segments; keep references to segments_all, segment_years, seg.end_year,
seg.start_year, all_years and CourseOccasion to locate the change.
In `@upgrade_remove_incremental_years.py`:
- Around line 99-101: The loop over CourseScheduleSegment accesses
segment.time_period.week without a null check; update the migration to first
check whether segment.time_period is not None before reading .week (or set
segment.start = None if time_period is missing), and only then call
segment.save(update_fields=["start"]); reference CourseScheduleSegment,
segment.time_period, segment.start and the save(update_fields=["start"]) call
when making the change.
- Around line 94-97: The loop in CourseOccasion
(CourseOccasion.objects.select_related("academic_year", "time_period"))
dereferences occasion.academic_year.year and occasion.time_period.week without
null checks; update the migration to check for None on occasion.academic_year
and occasion.time_period before reading their attributes and only set
occasion.year and/or occasion.start (and call
occasion.save(update_fields=[...])) for the fields that actually have source
values (or skip that record) to avoid AttributeError during migration.
- Around line 90-102: The migration's data copy function
copy_time_period_and_academic_year_data makes the transformation irreversible
because the migration uses migrations.RunPython.noop as the reverse; update the
migration file to add a clear top-of-file or above the RunPython call comment
stating that this migration is one-way (academic_year and time_period FK values
will not be restored on rollback) and that backups must be taken before applying
to production, and ensure the RunPython invocation still references
copy_time_period_and_academic_year_data and migrations.RunPython.noop so
reviewers can locate the change.
---
Outside diff comments:
In `@rodatraden/forms.py`:
- Around line 187-216: The year choices generation (year_choices) currently only
includes self.instance.start_year and end_year, so existing blacklisted years
outside that range can be lost; update the include_years value passed to
_dynamic_year_choices to also include any years from
self.instance.blacklisted_years when self.instance.pk is truthy (merge
start_year, end_year and self.instance.blacklisted_years, deduplicate and sort),
then build year_choices from that combined list so
self.fields['blacklisted_years'].choices and the initial population
(self.initial['blacklisted_years']) can retain and show those years as
selectable options.
- Around line 243-270: The cloned courseoccasion's year and start get lost
because you build year_choices with include_years based only on
self.instance.year (which is empty for a clone) and you replace the model-backed
start field with StartWeekField after setting its initial; fix by ensuring the
copied year is included in the dynamic choices and the copied start is applied
to the final StartWeekField: when computing year_choices, pass
include_years=[self.instance.year] if editing else
include_years=[courseocc_cpy.year] when 'courseocc' present (or merge both), and
either create/assign self.fields['start'] = StartWeekField(...) before you set
courseocc_cpy.start on self.fields['start'].initial or set
self.fields['start'].initial after instantiating StartWeekField so the copied
start value persists. Ensure you still set self.fields['year'].initial =
courseocc_cpy.year after adjusting choices.
In `@rodatraden/rodatraden_modules/functions.py`:
- Around line 42-49: Replace the bare except that swallows all errors when
resolving CourseOccasion with explicit exception handling: catch
CourseOccasion.DoesNotExist to skip missing mappings and optionally catch
CourseOccasion.MultipleObjectsReturned to handle duplicates (e.g., log or choose
one) and re-raise or log any other unexpected exceptions instead of silently
passing; update the block around the CourseOccasion.objects.get call that
populates new_course_occasions (referencing course_occasion and new_year) to use
these specific exception handlers so only expected lookup misses are ignored.
In `@rodatraden/views.py`:
- Around line 103-105: The view currently gates access by checking
request.user.is_staff and redirects with return redirect(reverse('index'));
update the access check to also allow users with the new permission by testing
request.user.has_perm('rodatraden.can_manage_scheduling') (i.e., replace or
augment the is_staff check with a logical OR so users who have
can_manage_scheduling are permitted); keep the existing
redirect(reverse('index')) behavior for users who lack both the staff flag and
the can_manage_scheduling permission.
---
Nitpick comments:
In `@rodatraden/management/commands/generate_course_occasions.py`:
- Around line 77-84: The period-number calculation (period_number =
segment.start // weeks_in_period + 1) is duplicated; replace this inline logic
by calling a shared helper or the model property to avoid duplication—either
extract a utility like compute_period_number(start, weeks_in_period) and use it
here and in infer_course_scheduling.py, or use the existing
CourseOccasion.period_label()/period-related property from models.py (and
reference weeks_in_period as the canonical constant) so both commands derive the
period label from the same single implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50982e5b-ae87-4a67-9f82-67a1cf5ca2ad
📒 Files selected for processing (23)
MIGRATION_GUIDE.mdREADME.mdrodatraden/admin.pyrodatraden/filters.pyrodatraden/forms.pyrodatraden/management/commands/generate_course_occasions.pyrodatraden/management/commands/infer_course_scheduling.pyrodatraden/management/commands/validate_course_schedule_parity.pyrodatraden/models.pyrodatraden/rodatraden_modules/forms.pyrodatraden/rodatraden_modules/functions.pyrodatraden/tables.pyrodatraden/templates/rodatraden/base.htmlrodatraden/templates/rodatraden/course/course_detail.htmlrodatraden/templates/rodatraden/courseoccasion/courseoccasion_confirm_delete.htmlrodatraden/templates/rodatraden/courseoccasion/courseoccasion_detail.htmlrodatraden/templates/rodatraden/courseoccasion/courseoccasion_info.htmlrodatraden/templates/rodatraden/courseoccasion/courseoccasion_list.htmlrodatraden/templates/rodatraden/rt_modal/course_list_table_row.htmlrodatraden/templates/rodatraden/tables/courseoccasion_edit.htmlrodatraden/templates/rodatraden/tools.htmlrodatraden/views.pyupgrade_remove_incremental_years.py
💤 Files with no reviewable changes (2)
- rodatraden/admin.py
- rodatraden/templates/rodatraden/base.html
Comment on lines
+81
to
88
| time_period = django_filters.ChoiceFilter( | ||
| choices=lambda: [('', 'Läsperiod')] + [ | ||
| (i * 10, f'LP{i}') for i in range(1, 6) | ||
| ], | ||
| empty_label=None, | ||
| field_name='start', | ||
| label='Läsperiod', | ||
| ) |
There was a problem hiding this comment.
Filter start by LP window, not exact week value.
The new start field stores offsets inside the period (13 means LP2 + 3 weeks), so an exact field_name='start' filter only finds occasions that start on week 10/20/30/40. The hardcoded values are also off by one period: LP1 should map to 0, and LP5 should map to 40.
Suggested fix
time_period = django_filters.ChoiceFilter(
choices=lambda: [('', 'Läsperiod')] + [
- (i * 10, f'LP{i}') for i in range(1, 6)
+ ((i - 1) * 10, f'LP{i}') for i in range(1, 6)
],
empty_label=None,
- field_name='start',
+ method='filter_time_period',
label='Läsperiod',
)
+
+ def filter_time_period(self, queryset, name, value):
+ if value in (None, ''):
+ return queryset
+ period_start = int(value)
+ return queryset.filter(
+ start__gte=period_start,
+ start__lt=period_start + 10,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| time_period = django_filters.ChoiceFilter( | |
| choices=lambda: [('', 'Läsperiod')] + [ | |
| (i * 10, f'LP{i}') for i in range(1, 6) | |
| ], | |
| empty_label=None, | |
| field_name='start', | |
| label='Läsperiod', | |
| ) | |
| time_period = django_filters.ChoiceFilter( | |
| choices=lambda: [('', 'Läsperiod')] + [ | |
| ((i - 1) * 10, f'LP{i}') for i in range(1, 6) | |
| ], | |
| empty_label=None, | |
| method='filter_time_period', | |
| label='Läsperiod', | |
| ) | |
| def filter_time_period(self, queryset, name, value): | |
| if value in (None, ''): | |
| return queryset | |
| period_start = int(value) | |
| return queryset.filter( | |
| start__gte=period_start, | |
| start__lt=period_start + 10, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/filters.py` around lines 81 - 88, The filter is currently matching
exact start weeks and uses wrong LP values; change the ChoiceFilter named
time_period to emit LP base offsets as (i-1)*10 (so LP1 -> 0, LP5 -> 40) and
switch it to a method filter (add method='filter_time_period') instead of
field_name='start' so you can return occasions whose start falls inside the LP
window (start between base and base+9). Implement a FilterSet method
filter_time_period(self, queryset, name, value) that parses the chosen base
offset, computes base=int(value) and filters queryset.filter(start__gte=base,
start__lte=base+9) (or start__lt=base+10) and leave the human label choices as
before.
Comment on lines
+115
to
+124
| for courseocc in CourseOccasion.objects.filter(year=from_year): | ||
| # Only create a new for the new year if it does not already | ||
| # exist | ||
| if not CourseOccasion.objects.filter( | ||
| year=to_year, | ||
| course=courseocc.course): | ||
| courseocc.pk = None | ||
| courseocc.year = to_year | ||
| courseocc.slug = '' | ||
| courseocc.save() |
There was a problem hiding this comment.
Copy each distinct occasion, not just one per course/year.
The existence check only keys on course and year. If a course has multiple occasions in the source year, the first copied row causes every later one to be skipped, so LP1/LP3-style duplicates collapse into a single target occasion.
Suggested fix
- if not CourseOccasion.objects.filter(
- year=to_year,
- course=courseocc.course):
+ if not CourseOccasion.objects.filter(
+ year=to_year,
+ course=courseocc.course,
+ start=courseocc.start,
+ ).exists():
courseocc.pk = None
courseocc.year = to_year
courseocc.slug = ''
courseocc.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/views.py` around lines 115 - 124, The current loop in
CourseOccasion copying uses an existence check that only filters by year and
course, causing multiple distinct source occasions to collapse into one; update
the check in the block that references CourseOccasion and courseocc so it
compares all fields that make an occasion distinct (for example include
start_date/start_time, end_time, location, and any other unique keys) such that
you do: CourseOccasion.objects.filter(year=to_year, course=courseocc.course,
start_date=courseocc.start_date, start_time=courseocc.start_time,
location=courseocc.location) (or the equivalent set of unique fields for your
model) before skipping creation, ensuring each distinct source occasion is
copied.
Comment on lines
+156
to
+169
| # Determine the year range to check: cover all segment ranges plus any | ||
| # existing occasions, and extend up to current year + 10 for future gen. | ||
| segments_all = course.schedule_segments.all() | ||
| segment_years = set() | ||
| for seg in segments_all: | ||
| end = seg.end_year or seg.start_year | ||
| segment_years.update(range(seg.start_year, end + 1)) | ||
|
|
||
| existing_years = set( | ||
| CourseOccasion.objects.filter(course=course) | ||
| .values_list('year', flat=True) | ||
| ) | ||
| current = datetime.date.today().year | ||
| all_years = segment_years | existing_years | set(range(current, current + 11)) |
There was a problem hiding this comment.
Open-ended segments currently skip historical years.
For end_year=None, Line 161 collapses the segment span to start_year only. all_years then adds current..current+10, but it never covers the gap between start_year + 1 and current - 1, so applying scheduling rules cannot recreate missing past occasions for long-running courses.
Suggested fix
- segments_all = course.schedule_segments.all()
- segment_years = set()
+ current = datetime.date.today().year
+ future_end = current + YEAR_RANGE_OFFSET
+ segments_all = course.schedule_segments.all()
+ segment_years = set()
for seg in segments_all:
- end = seg.end_year or seg.start_year
- segment_years.update(range(seg.start_year, end + 1))
+ effective_end = seg.end_year or future_end
+ segment_years.update(range(seg.start_year, effective_end + 1))
existing_years = set(
CourseOccasion.objects.filter(course=course)
.values_list('year', flat=True)
)
- current = datetime.date.today().year
- all_years = segment_years | existing_years | set(range(current, current + 11))
+ all_years = segment_years | existing_years | set(range(current, future_end + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/views.py` around lines 156 - 169, The loop building segment_years
treats seg.end_year None as seg.start_year only, skipping historical years for
open-ended segments; compute current = datetime.date.today().year before the
loop and when iterating segments_all set end = seg.end_year if present else
current - 1 (and ensure end = max(end, seg.start_year)), then update
segment_years with range(seg.start_year, end + 1) so all_years includes past
years for open-ended segments; keep references to segments_all, segment_years,
seg.end_year, seg.start_year, all_years and CourseOccasion to locate the change.
Comment on lines
+90
to
+102
| def copy_time_period_and_academic_year_data(apps, schema_editor): | ||
| CourseOccasion = apps.get_model("rodatraden", "CourseOccasion") | ||
| CourseScheduleSegment = apps.get_model("rodatraden", "CourseScheduleSegment") | ||
|
|
||
| for occasion in CourseOccasion.objects.select_related("academic_year", "time_period").all(): | ||
| occasion.year = occasion.academic_year.year | ||
| occasion.start = occasion.time_period.week | ||
| occasion.save(update_fields=["year", "start"]) | ||
|
|
||
| for segment in CourseScheduleSegment.objects.select_related("time_period").all(): | ||
| segment.start = segment.time_period.week | ||
| segment.save(update_fields=["start"]) | ||
|
|
There was a problem hiding this comment.
Data migration is irreversible — rollback will lose data.
The RunPython operation uses migrations.RunPython.noop as the reverse, meaning a rollback won't restore academic_year and time_period FK values. This is acceptable for a one-way upgrade, but ensure backups are taken before running this migration in production.
📝 Suggestion: Add explicit comment about irreversibility
- migrations.RunPython(copy_time_period_and_academic_year_data, migrations.RunPython.noop),
+ # CAUTION: Irreversible — backup database before running
+ migrations.RunPython(copy_time_period_and_academic_year_data, migrations.RunPython.noop),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade_remove_incremental_years.py` around lines 90 - 102, The migration's
data copy function copy_time_period_and_academic_year_data makes the
transformation irreversible because the migration uses migrations.RunPython.noop
as the reverse; update the migration file to add a clear top-of-file or above
the RunPython call comment stating that this migration is one-way (academic_year
and time_period FK values will not be restored on rollback) and that backups
must be taken before applying to production, and ensure the RunPython invocation
still references copy_time_period_and_academic_year_data and
migrations.RunPython.noop so reviewers can locate the change.
Comment on lines
+94
to
+97
| for occasion in CourseOccasion.objects.select_related("academic_year", "time_period").all(): | ||
| occasion.year = occasion.academic_year.year | ||
| occasion.start = occasion.time_period.week | ||
| occasion.save(update_fields=["year", "start"]) |
There was a problem hiding this comment.
Verify source fields exist before accessing.
The data migration accesses occasion.academic_year.year and occasion.time_period.week without null checks. If any occasion has a null FK, this will raise an AttributeError.
🐛 Proposed fix: Add null guards
for occasion in CourseOccasion.objects.select_related("academic_year", "time_period").all():
- occasion.year = occasion.academic_year.year
- occasion.start = occasion.time_period.week
- occasion.save(update_fields=["year", "start"])
+ if occasion.academic_year and occasion.time_period:
+ occasion.year = occasion.academic_year.year
+ occasion.start = occasion.time_period.week
+ occasion.save(update_fields=["year", "start"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for occasion in CourseOccasion.objects.select_related("academic_year", "time_period").all(): | |
| occasion.year = occasion.academic_year.year | |
| occasion.start = occasion.time_period.week | |
| occasion.save(update_fields=["year", "start"]) | |
| for occasion in CourseOccasion.objects.select_related("academic_year", "time_period").all(): | |
| if occasion.academic_year and occasion.time_period: | |
| occasion.year = occasion.academic_year.year | |
| occasion.start = occasion.time_period.week | |
| occasion.save(update_fields=["year", "start"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade_remove_incremental_years.py` around lines 94 - 97, The loop in
CourseOccasion (CourseOccasion.objects.select_related("academic_year",
"time_period")) dereferences occasion.academic_year.year and
occasion.time_period.week without null checks; update the migration to check for
None on occasion.academic_year and occasion.time_period before reading their
attributes and only set occasion.year and/or occasion.start (and call
occasion.save(update_fields=[...])) for the fields that actually have source
values (or skip that record) to avoid AttributeError during migration.
Comment on lines
+99
to
+101
| for segment in CourseScheduleSegment.objects.select_related("time_period").all(): | ||
| segment.start = segment.time_period.week | ||
| segment.save(update_fields=["start"]) |
There was a problem hiding this comment.
Same null check needed for CourseScheduleSegment.
The segment migration also accesses segment.time_period.week without a null guard.
🐛 Proposed fix: Add null guard
for segment in CourseScheduleSegment.objects.select_related("time_period").all():
- segment.start = segment.time_period.week
- segment.save(update_fields=["start"])
+ if segment.time_period:
+ segment.start = segment.time_period.week
+ segment.save(update_fields=["start"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for segment in CourseScheduleSegment.objects.select_related("time_period").all(): | |
| segment.start = segment.time_period.week | |
| segment.save(update_fields=["start"]) | |
| for segment in CourseScheduleSegment.objects.select_related("time_period").all(): | |
| if segment.time_period: | |
| segment.start = segment.time_period.week | |
| segment.save(update_fields=["start"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade_remove_incremental_years.py` around lines 99 - 101, The loop over
CourseScheduleSegment accesses segment.time_period.week without a null check;
update the migration to first check whether segment.time_period is not None
before reading .week (or set segment.start = None if time_period is missing),
and only then call segment.save(update_fields=["start"]); reference
CourseScheduleSegment, segment.time_period, segment.start and the
save(update_fields=["start"]) call when making the change.
…ncorporate master improvements - Resolve 7 content conflicts (filters.py, forms.py, models.py, rodatraden_modules/forms.py, courseoccasion_info.html, tools.html, views.py) - Accept master deletions: MIGRATION_GUIDE.md, courseoccasion_list.html - Keep HEAD model changes: start IntegerField on CourseOccasion/CourseScheduleSegment, no AcademicYear/TimePeriod FK - Incorporate master UX: block-lookup for in_current_block in courseoccasion_info, collapsed Ger behörighet section, remove legacy copy tool from tools view - Remove orphaned start_offset field and duplicate start_string from CourseScheduleSegment - Fix static asset updates from master (D3 v7, Chart.js v4, jQuery 4, select2 4.0.13) Co-authored-by: SpiderQubit <25958778+SpiderQubit@users.noreply.github.com>
Resolved in merge commit
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
rodatraden/views.py (1)
131-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpen-ended segments still skip the years between
start_yearand today.For
end_year=None, Line 136 still collapses the segment span tostart_yearonly. The latercurrent..current+10union adds future years, but it never backfills missing years in between, so applying scheduling rules cannot recreate historical occasions for long-running courses.🗓️ Suggested fix
- segments_all = course.schedule_segments.all() - segment_years = set() + current = datetime.date.today().year + future_end = current + YEAR_RANGE_OFFSET + segments_all = course.schedule_segments.all() + segment_years = set() for seg in segments_all: - end = seg.end_year or seg.start_year - segment_years.update(range(seg.start_year, end + 1)) + end = seg.end_year if seg.end_year is not None else future_end + segment_years.update(range(seg.start_year, max(end, seg.start_year) + 1)) @@ - current = datetime.date.today().year - all_years = segment_years | existing_years | set(range(current, current + 11)) + all_years = segment_years | existing_years | set(range(current, future_end + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/views.py` around lines 131 - 145, The segment year calculation collapses open-ended segments because seg.end_year is replaced with seg.start_year; fix by using the current year when seg.end_year is None: compute current = datetime.date.today().year before building segment_years, then set end = seg.end_year or current and update segment_years with range(seg.start_year, end + 1); leave the later union with future years (all_years) unchanged so historical years between start_year and today are backfilled for CourseOccasion/scheduling logic.rodatraden/filters.py (1)
80-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter LP windows instead of exact week numbers.
startnow stores offsets inside the period (13= LP2 + 3 weeks), sofield_name='start'only matches occasions that start on week10/20/30/40. The choice values are also shifted by one period: LP1 should emit0, not10. This still breaks the Läsperiod filter for most generated occasions.🔎 Suggested fix
time_period = django_filters.ChoiceFilter( choices=lambda: [('', 'Läsperiod')] + [ - (i * 10, f'LP{i}') for i in range(1, 6) + ((i - 1) * 10, f'LP{i}') for i in range(1, 6) ], empty_label=None, - field_name='start', + method='filter_time_period', label='Läsperiod', ) + + def filter_time_period(self, queryset, name, value): + if value in (None, ''): + return queryset + period_start = int(value) + return queryset.filter( + start__gte=period_start, + start__lt=period_start + 10, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rodatraden/filters.py` around lines 80 - 87, The current time_period ChoiceFilter uses field_name='start' and choice values that are shifted; instead, make choices emit period bases (LP1 -> 0, LP2 -> 10, etc.) and use a custom method to match the LP window (start offsets) rather than exact week numbers: change choices to lambda: [('', 'Läsperiod')] + [((i-1)*10, f'LP{i}') for i in range(1,6)] and replace field_name='start' with method='filter_time_period', then implement a filter_time_period(self, queryset, name, value) that returns the queryset unmodified when value is empty and otherwise filters with queryset.filter(start__gte=value, start__lt=value+10) so occasions whose start offset falls within the selected LP are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 177-186: This section documents the post-refactor data model but
omits a critical, mandatory upgrade step; update the paragraph after the
admin/setup instructions to explicitly call out and instruct users to run the
upgrade script `python upgrade_remove_incremental_years.py` before running
migrations (i.e., before following the generic `makemigrations`/`migrate` flow),
making clear it is required for existing environments and where it should be
executed in the setup sequence.
In `@rodatraden/forms.py`:
- Around line 253-280: The copied CourseOccasion's start initial is being lost
because you assign self.fields['start'].initial from courseocc_cpy, then
immediately replace that field with a new StartWeekField at the end; either
instantiate the StartWeekField before you apply the copied values or
(preferably) move the line self.fields['start'] =
StartWeekField(label='Läsperiod') to above the "if 'courseocc' in
self.request.GET" block so the initial on self.fields['start'] persists, or
alternatively set self.fields['start'].initial = courseocc_cpy.start after
creating the StartWeekField; update the code around StartWeekField and the copy
block accordingly (references: StartWeekField and self.fields['start'], and the
courseocc_cpy assignment).
In `@rodatraden/models.py`:
- Around line 511-515: Add explicit model-level and field-level bounds for the
start week: on the IntegerField named 'start' in the model that currently
defines start = models.IntegerField(...), add
django.core.validators.MinValueValidator(0) and MaxValueValidator(49) (or change
to PositiveSmallIntegerField with those validators) and also add a model
CheckConstraint enforcing 0 <= start <= 49 on the containing model; apply the
same validators and a matching CheckConstraint to the other week-index
IntegerField(s) elsewhere in the file that represent week indices so
out-of-range values cannot be written by fixtures/admin/scripts.
- Around line 658-660: CourseOccasion.__str__ currently returns only
self.course.title + ' - ' + str(self.year), which makes multiple occasions in
the same year ambiguous; change the method to include the occasion period (e.g.,
append ' - ' + str(self.period) or the appropriate period attribute) so the
string contains self.course.title, self.year and self.period (reference
CourseOccasion.__str__, self.course.title, self.year, self.period).
In `@rodatraden/views.py`:
- Around line 1515-1526: The query is excluding occasions already in the target
block so co.in_block can never be true; remove the .exclude(block__id=block.id)
from the CourseOccasion.objects query (the code that builds courseoccasions) so
the filter(year=..., start__gte=..., start__lt=...) returns all occasions in the
window, then rely on the existing logic that sets/reads co.in_block to mark
which ones belong to the current block.
---
Duplicate comments:
In `@rodatraden/filters.py`:
- Around line 80-87: The current time_period ChoiceFilter uses
field_name='start' and choice values that are shifted; instead, make choices
emit period bases (LP1 -> 0, LP2 -> 10, etc.) and use a custom method to match
the LP window (start offsets) rather than exact week numbers: change choices to
lambda: [('', 'Läsperiod')] + [((i-1)*10, f'LP{i}') for i in range(1,6)] and
replace field_name='start' with method='filter_time_period', then implement a
filter_time_period(self, queryset, name, value) that returns the queryset
unmodified when value is empty and otherwise filters with
queryset.filter(start__gte=value, start__lt=value+10) so occasions whose start
offset falls within the selected LP are returned.
In `@rodatraden/views.py`:
- Around line 131-145: The segment year calculation collapses open-ended
segments because seg.end_year is replaced with seg.start_year; fix by using the
current year when seg.end_year is None: compute current =
datetime.date.today().year before building segment_years, then set end =
seg.end_year or current and update segment_years with range(seg.start_year, end
+ 1); leave the later union with future years (all_years) unchanged so
historical years between start_year and today are backfilled for
CourseOccasion/scheduling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d03b4b98-3134-4415-9329-9024493e00e3
📒 Files selected for processing (8)
README.mdrodatraden/filters.pyrodatraden/forms.pyrodatraden/models.pyrodatraden/tables.pyrodatraden/templates/rodatraden/courseoccasion/courseoccasion_detail.htmlrodatraden/templates/rodatraden/courseoccasion/courseoccasion_info.htmlrodatraden/views.py
✅ Files skipped from review due to trivial changes (2)
- rodatraden/templates/rodatraden/courseoccasion/courseoccasion_info.html
- rodatraden/tables.py
🚧 Files skipped from review as they are similar to previous changes (1)
- rodatraden/templates/rodatraden/courseoccasion/courseoccasion_detail.html
Comment on lines
+177
to
+186
| After this navigate to the admin interface. Here we will have to add entries like "institutioner" "Nivåer" and "Spår". | ||
| - Institutioner : The institution / faculty where the course can be / is taken | ||
| - Nivåer : If it's an advanced course or not (grundläggande / avancerad) | ||
| - Spår : The different tracks that can be choosen. Make a profile before on the main site | ||
| - Tidsperioder : This is when during the year a course starts. For example week 0 for "läsperiod 1", week 10 for "läsperiod 2" Week 0 is the beginning of time academic year. If a course starts 3 weeks into period 2, then specify week 13. | ||
| Course periods are now entered directly as start week values in forms | ||
| (LP1=0, LP2=10, LP3=20, LP4=30, LP5=40). If a course starts 3 weeks into | ||
| period 2, set start week to 13. | ||
|
|
||
| Academic years (e.g. "20/21") are computed automatically — no database setup | ||
| needed. Year dropdowns in forms show the current year ± 10. |
There was a problem hiding this comment.
Call out the mandatory upgrade script in this section.
This new setup guidance explains the post-refactor data model, but it still leaves upgrade readers one scroll away from the generic makemigrations instructions above. For this PR, that is the wrong path: existing environments need python upgrade_remove_incremental_years.py, otherwise the migration flow is easy to run incorrectly.
📝 Suggested wording
After this navigate to the admin interface. Here we will have to add entries like "institutioner" "Nivåer" and "Spår".
- Institutioner : The institution / faculty where the course can be / is taken
- Nivåer : If it's an advanced course or not (grundläggande / avancerad)
- Spår : The different tracks that can be choosen. Make a profile before on the main site
+
+> **Upgrade note:** Existing installations must run
+> `python upgrade_remove_incremental_years.py` in each environment for this
+> change set. Do not rely on plain `python manage.py makemigrations` when
+> upgrading from the old AcademicYear/TimePeriod model.
Course periods are now entered directly as start week values in forms
(LP1=0, LP2=10, LP3=20, LP4=30, LP5=40). If a course starts 3 weeks into
period 2, set start week to 13.🧰 Tools
🪛 LanguageTool
[style] ~179-~179: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...the course can be / is taken - Nivåer : If it's an advanced course or not (grundlä...
(IF_WHETHER)
[grammar] ~180-~180: Ensure spelling is correct
Context: ...Spår : The different tracks that can be choosen. Make a profile before on the main site...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 177 - 186, This section documents the post-refactor
data model but omits a critical, mandatory upgrade step; update the paragraph
after the admin/setup instructions to explicitly call out and instruct users to
run the upgrade script `python upgrade_remove_incremental_years.py` before
running migrations (i.e., before following the generic
`makemigrations`/`migrate` flow), making clear it is required for existing
environments and where it should be executed in the setup sequence.
Comment on lines
+253
to
+280
| # Year is now a plain IntegerField — present as a dropdown with | ||
| # dynamically computed year choices (current year ± 10). | ||
| year_choices = _dynamic_year_choices( | ||
| include_years=[self.instance.year] if self.instance.pk else None | ||
| ) | ||
| self.fields['year'] = forms.ChoiceField( | ||
| choices=year_choices, | ||
| initial=datetime.datetime.now().year, | ||
| label='År', | ||
| ) | ||
|
|
||
| # If a courseoccasion is copied | ||
| if 'courseocc' in self.request.GET: | ||
| courseocc_cpy = CourseOccasion.objects.get(slug=self.request.GET['courseocc']) | ||
| # This hard-coding might be avoidable. Not sure... | ||
| self.fields['course'].initial = courseocc_cpy.course | ||
| self.fields['academic_year'].initial = courseocc_cpy.academic_year | ||
| self.fields['start_week'].initial = courseocc_cpy.time_period.week | ||
| self.fields['year'].initial = courseocc_cpy.year | ||
| self.fields['start'].initial = courseocc_cpy.start | ||
| self.fields['weeks'].initial = courseocc_cpy.weeks | ||
| self.fields['note'].initial = courseocc_cpy.note | ||
| self.fields['contact_name'].initial = courseocc_cpy.contact_name | ||
| self.fields['contact_email'].initial = courseocc_cpy.contact_email | ||
| self.fields['official'].initial = courseocc_cpy.official | ||
|
|
||
| # Populate start week when editing an existing occasion. | ||
| if self.instance.pk: | ||
| self.fields['start_week'].initial = self.instance.time_period.week | ||
|
|
||
| self.fields['course'].widget.attrs['class'] = \ | ||
| 'course-list-filter-courseocc-create' | ||
|
|
||
| self.order_fields([ | ||
| 'course', | ||
| 'academic_year', | ||
| 'start_week', | ||
| 'weeks', | ||
| 'note', | ||
| 'contact_name', | ||
| 'contact_email', | ||
| 'official', | ||
| ]) | ||
|
|
||
| @staticmethod | ||
| def _format_time_period_title(start_week: int) -> str: | ||
| weeks_in_period = 10 | ||
| period_number = start_week // weeks_in_period + 1 | ||
| period_start_offset = start_week % weeks_in_period | ||
|
|
||
| result = f'LP{period_number}' | ||
| if period_start_offset: | ||
| postfix = 'vecka' if period_start_offset == 1 else 'veckor' | ||
| result += f' - {period_start_offset} {postfix} in' | ||
| return result | ||
|
|
||
| def save(self, commit=True): | ||
| courseoccasion = super().save(commit=False) | ||
| start_week = self.cleaned_data['start_week'] | ||
|
|
||
| # Resolve existing period by week, or create one if it is missing. | ||
| time_period = TimePeriod.objects.filter(week=start_week).first() | ||
| if time_period is None: | ||
| time_period = TimePeriod.objects.create( | ||
| week=start_week, | ||
| title=self._format_time_period_title(start_week), | ||
| ) | ||
| self.fields['start'] = StartWeekField(label='Läsperiod') |
There was a problem hiding this comment.
Don't overwrite the copied start initial.
In the copy flow, Line 270 sets the initial value on the current start field, but Line 280 immediately replaces that field with a new StartWeekField. The copied occasion therefore loses its start period and falls back to the default widget state.
💡 Minimal fix
+ self.fields['start'] = StartWeekField(label='Läsperiod')
+
# If a courseoccasion is copied
if 'courseocc' in self.request.GET:
courseocc_cpy = CourseOccasion.objects.get(slug=self.request.GET['courseocc'])
# This hard-coding might be avoidable. Not sure...
self.fields['course'].initial = courseocc_cpy.course
self.fields['year'].initial = courseocc_cpy.year
- self.fields['start'].initial = courseocc_cpy.start
+ self.initial['start'] = courseocc_cpy.start
self.fields['weeks'].initial = courseocc_cpy.weeks
self.fields['note'].initial = courseocc_cpy.note
self.fields['contact_name'].initial = courseocc_cpy.contact_name
self.fields['contact_email'].initial = courseocc_cpy.contact_email
self.fields['official'].initial = courseocc_cpy.official
@@
- self.fields['start'] = StartWeekField(label='Läsperiod')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Year is now a plain IntegerField — present as a dropdown with | |
| # dynamically computed year choices (current year ± 10). | |
| year_choices = _dynamic_year_choices( | |
| include_years=[self.instance.year] if self.instance.pk else None | |
| ) | |
| self.fields['year'] = forms.ChoiceField( | |
| choices=year_choices, | |
| initial=datetime.datetime.now().year, | |
| label='År', | |
| ) | |
| # If a courseoccasion is copied | |
| if 'courseocc' in self.request.GET: | |
| courseocc_cpy = CourseOccasion.objects.get(slug=self.request.GET['courseocc']) | |
| # This hard-coding might be avoidable. Not sure... | |
| self.fields['course'].initial = courseocc_cpy.course | |
| self.fields['academic_year'].initial = courseocc_cpy.academic_year | |
| self.fields['start_week'].initial = courseocc_cpy.time_period.week | |
| self.fields['year'].initial = courseocc_cpy.year | |
| self.fields['start'].initial = courseocc_cpy.start | |
| self.fields['weeks'].initial = courseocc_cpy.weeks | |
| self.fields['note'].initial = courseocc_cpy.note | |
| self.fields['contact_name'].initial = courseocc_cpy.contact_name | |
| self.fields['contact_email'].initial = courseocc_cpy.contact_email | |
| self.fields['official'].initial = courseocc_cpy.official | |
| # Populate start week when editing an existing occasion. | |
| if self.instance.pk: | |
| self.fields['start_week'].initial = self.instance.time_period.week | |
| self.fields['course'].widget.attrs['class'] = \ | |
| 'course-list-filter-courseocc-create' | |
| self.order_fields([ | |
| 'course', | |
| 'academic_year', | |
| 'start_week', | |
| 'weeks', | |
| 'note', | |
| 'contact_name', | |
| 'contact_email', | |
| 'official', | |
| ]) | |
| @staticmethod | |
| def _format_time_period_title(start_week: int) -> str: | |
| weeks_in_period = 10 | |
| period_number = start_week // weeks_in_period + 1 | |
| period_start_offset = start_week % weeks_in_period | |
| result = f'LP{period_number}' | |
| if period_start_offset: | |
| postfix = 'vecka' if period_start_offset == 1 else 'veckor' | |
| result += f' - {period_start_offset} {postfix} in' | |
| return result | |
| def save(self, commit=True): | |
| courseoccasion = super().save(commit=False) | |
| start_week = self.cleaned_data['start_week'] | |
| # Resolve existing period by week, or create one if it is missing. | |
| time_period = TimePeriod.objects.filter(week=start_week).first() | |
| if time_period is None: | |
| time_period = TimePeriod.objects.create( | |
| week=start_week, | |
| title=self._format_time_period_title(start_week), | |
| ) | |
| self.fields['start'] = StartWeekField(label='Läsperiod') | |
| # Year is now a plain IntegerField — present as a dropdown with | |
| # dynamically computed year choices (current year ± 10). | |
| year_choices = _dynamic_year_choices( | |
| include_years=[self.instance.year] if self.instance.pk else None | |
| ) | |
| self.fields['year'] = forms.ChoiceField( | |
| choices=year_choices, | |
| initial=datetime.datetime.now().year, | |
| label='År', | |
| ) | |
| self.fields['start'] = StartWeekField(label='Läsperiod') | |
| # If a courseoccasion is copied | |
| if 'courseocc' in self.request.GET: | |
| courseocc_cpy = CourseOccasion.objects.get(slug=self.request.GET['courseocc']) | |
| # This hard-coding might be avoidable. Not sure... | |
| self.fields['course'].initial = courseocc_cpy.course | |
| self.fields['year'].initial = courseocc_cpy.year | |
| self.initial['start'] = courseocc_cpy.start | |
| self.fields['weeks'].initial = courseocc_cpy.weeks | |
| self.fields['note'].initial = courseocc_cpy.note | |
| self.fields['contact_name'].initial = courseocc_cpy.contact_name | |
| self.fields['contact_email'].initial = courseocc_cpy.contact_email | |
| self.fields['official'].initial = courseocc_cpy.official | |
| self.fields['course'].widget.attrs['class'] = \ | |
| 'course-list-filter-courseocc-create' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/forms.py` around lines 253 - 280, The copied CourseOccasion's
start initial is being lost because you assign self.fields['start'].initial from
courseocc_cpy, then immediately replace that field with a new StartWeekField at
the end; either instantiate the StartWeekField before you apply the copied
values or (preferably) move the line self.fields['start'] =
StartWeekField(label='Läsperiod') to above the "if 'courseocc' in
self.request.GET" block so the initial on self.fields['start'] persists, or
alternatively set self.fields['start'].initial = courseocc_cpy.start after
creating the StartWeekField; update the code around StartWeekField and the copy
block accordingly (references: StartWeekField and self.fields['start'], and the
courseocc_cpy assignment).
Comment on lines
+511
to
515
| start = models.IntegerField( | ||
| verbose_name='Läsperiod', | ||
| help_text='Startvecka för perioden (0 = LP1, 10 = LP2, 20 = LP3, 30 = LP4, 40 = LP5)', | ||
| default=0, | ||
| ) |
There was a problem hiding this comment.
Add model-level bounds for start before bad data reaches scheduling logic.
StartWeekField constrains form input, but these raw IntegerFields can still be written from fixtures, admin actions, scripts, or the custom upgrade path. Values outside 0..49 would produce invalid LP0/LP6 labels and fall outside the filter/generation assumptions elsewhere in this PR.
🛡️ Suggested fix
+from django.core.validators import MaxValueValidator, MinValueValidator
@@
start = models.IntegerField(
verbose_name='Läsperiod',
help_text='Startvecka för perioden (0 = LP1, 10 = LP2, 20 = LP3, 30 = LP4, 40 = LP5)',
default=0,
+ validators=[MinValueValidator(0), MaxValueValidator(49)],
)
@@
- start = models.IntegerField(verbose_name='Läsperiod', default=0)
+ start = models.IntegerField(
+ verbose_name='Läsperiod',
+ default=0,
+ validators=[MinValueValidator(0), MaxValueValidator(49)],
+ )Also applies to: 640-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/models.py` around lines 511 - 515, Add explicit model-level and
field-level bounds for the start week: on the IntegerField named 'start' in the
model that currently defines start = models.IntegerField(...), add
django.core.validators.MinValueValidator(0) and MaxValueValidator(49) (or change
to PositiveSmallIntegerField with those validators) and also add a model
CheckConstraint enforcing 0 <= start <= 49 on the containing model; apply the
same validators and a matching CheckConstraint to the other week-index
IntegerField(s) elsewhere in the file that represent week indices so
out-of-range values cannot be written by fixtures/admin/scripts.
Comment on lines
658
to
660
| def __str__(self): | ||
| return self.course.title + ' - ' + str(self.academic_year.year) | ||
| return self.course.title + ' - ' + str(self.year) | ||
|
|
There was a problem hiding this comment.
Include the period in CourseOccasion.__str__.
With automatic scheduling, a course can now legitimately have multiple occasions in the same academic year. Returning only "{title} - {year}" makes those rows indistinguishable in admin and any model-choice UI.
🧭 Suggested fix
- return self.course.title + ' - ' + str(self.year)
+ return f'{self.course.title} - {self.year_title} - {self.start_string}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/models.py` around lines 658 - 660, CourseOccasion.__str__
currently returns only self.course.title + ' - ' + str(self.year), which makes
multiple occasions in the same year ambiguous; change the method to include the
occasion period (e.g., append ' - ' + str(self.period) or the appropriate period
attribute) so the string contains self.course.title, self.year and self.period
(reference CourseOccasion.__str__, self.course.title, self.year, self.period).
Comment on lines
1515
to
1526
| # Get ALL course occasions starting in the given period, including those | ||
| # already in the block. Previously, already-in-block occasions were excluded | ||
| # which effectively limited retakes. Now they are shown but marked as | ||
| # "in_block" so the user can see all available options. | ||
| courseoccasions = CourseOccasion.objects.filter( | ||
| academic_year__year=year, | ||
| time_period__week__gte=start, | ||
| time_period__week__lt=start+10 | ||
| year=year, | ||
| start__gte=start, | ||
| start__lt=start+10 | ||
| ).exclude( | ||
| # Exclude course occasions already in THIS slot | ||
| block__id=block.id | ||
| ).order_by('course__title') |
There was a problem hiding this comment.
This query still hides the very occasions you're marking as in_block.
After filtering to the requested year/start window, exclude(block__id=block.id) removes every occasion already scheduled in that slot. That makes co.in_block permanently false and defeats the “show all options, mark already added” behavior described in the surrounding comments.
📋 Minimal fix
courseoccasions = CourseOccasion.objects.filter(
year=year,
start__gte=start,
start__lt=start+10
- ).exclude(
- # Exclude course occasions already in THIS slot
- block__id=block.id
).order_by('course__title')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rodatraden/views.py` around lines 1515 - 1526, The query is excluding
occasions already in the target block so co.in_block can never be true; remove
the .exclude(block__id=block.id) from the CourseOccasion.objects query (the code
that builds courseoccasions) so the filter(year=..., start__gte=...,
start__lt=...) returns all occasions in the window, then rely on the existing
logic that sets/reads co.in_block to mark which ones belong to the current
block.
SpiderQubit
marked this pull request as draft
May 2, 2026 09:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR removes the legacy
AcademicYearmodel and introduces an automatic course scheduling system based on reusable scheduling rules.Key Changes
New Features
CourseScheduleSegmentmodel – Define when courses are offered (period, frequency, year range, excluded years)/verktyg/) – Generate course occasions for all years across all coursesauto_generatedfield onCourseOccasionto manage automatically vs. manually created occasionscan_manage_schedulingpermission to restrict scheduling actionsRemoved
AcademicYearmodel – Replaced with plainIntegerFieldonCourseOccasionCourse.closedfield – Unused; removedensure_academic_yearscommand – No longer neededBenefits
Upgrade Instructions
Important: Do NOT use plain
makemigrationsfor this upgrade.For each environment, run the upgrade script: