fix: Ensure all eligible persons are assigned by implementing weighted priority logic in auto assignment #4932
Conversation
… feature/improved-assignments-distribution
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a data-driven assignment/autofill system: new conflict matrix and structured assignment constants; modules for candidate selection, assignment statistics, and a modular, language-group-aware autofill orchestrator; scoped schedule/history persistence and a person time‑away helper. (46 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as "useScheduleAutofill"
participant Orchestrator as "handleDynamicAssignmentAutofill"
participant Stats as "assignments_with_stats"
participant Selector as "assignment_selection"
participant Store as "schedules service"
Hook->>Orchestrator: start, end, languageGroups, meeting
Orchestrator->>Stats: getAssignmentsWithStats(persons, weeks, schedules, settings, languageGroups)
Stats-->>Orchestrator: frequencies, eligibleUIDs, benchmarks
Orchestrator->>Orchestrator: build & filter tasks per-week/view
Orchestrator->>Selector: sortCandidatesMultiLevel(candidates, task, history, metrics)
Selector-->>Orchestrator: orderedCandidates
Orchestrator->>Selector: hasAssignmentConflict(candidate, week, code, history, dataView)
Selector-->>Orchestrator: conflictDecision
Orchestrator->>Store: persist assignments (with dataView)
Store-->>Orchestrator: save result / update history
Orchestrator-->>Hook: completion or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… feature/improved-assignments-distribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@src/constants/index.ts`:
- Line 736: The Week.SPECIAL_TALK mapping currently constructs the Set
incorrectly using new Set(...ASSIGNMENT_PATH_KEYS) which spreads the array into
characters; replace it with new Set(ASSIGNMENT_PATH_KEYS) (i.e., remove the
spread operator) so the Set is constructed from the ASSIGNMENT_PATH_KEYS
iterable as intended; update the entry where Week.SPECIAL_TALK is paired with
new Set(...ASSIGNMENT_PATH_KEYS).
In `@src/features/meetings/week_range_selector/useWeekRangeSelector.tsx`:
- Around line 90-93: In useWeekRangeSelector, remove the hardcoded override that
sets startDate = '2024/11/01' inside the startWeekOptions block so the value
uses the computed result from getFirstWeekPreviousMonth() (formatted via
formatDate); update any related logic that assumed the override to rely on the
dynamic startDate instead (refer to startWeekOptions, startDate,
getFirstWeekPreviousMonth, and formatDate to locate the code).
In `@src/services/app/assignment_selection.ts`:
- Around line 139-147: The filter callback for computing pairings can throw when
assignment.key is undefined; update the check in the history.filter callback
(used to compute pairings) to guard against undefined by using optional chaining
or a null-safe test before calling includes on assignment.key (reference:
assignment.key, AssignmentHistoryType, pairings, history.filter). Ensure the
condition becomes something like a safe check (e.g.,
assignment.key?.includes('_Assistant_') or coercing to a string) so it returns
false when key is undefined and preserves the other checks for
entry.assignment.person and the student match.
- Around line 234-237: Protect against division by zero before computing
recoveryProgress: in assignment_selection.ts, where assistantWeightedWait is
divided by assignmentCodeThreshold (variables assistantWeightedWait,
assistantSafeDist, weightingFactor, recoveryProgress, assignmentCodeThreshold),
add a guard so if assignmentCodeThreshold is 0 (or <= 0) then set
recoveryProgress to 1 (matching the later capping behavior) instead of
performing the division; otherwise compute recoveryProgress =
assistantWeightedWait / assignmentCodeThreshold. This prevents Infinity when
assistantThreshold can be zero from
handleDynamicAssignmentAutofill/assistantThreshold.
In `@src/services/app/assignments_with_stats.ts`:
- Around line 551-554: The AssignmentMetrics.eligibleUIDS property expects a
Set<string> but eligiblePersonsView?.get(code) can be undefined; update the
statsForView.set call (in the block that constructs AssignmentMetrics) to supply
a default empty Set when eligiblePersonsView?.get(code) is missing (e.g.,
replace eligiblePersonsView?.get(code) with a fallback to new Set()). Ensure you
reference the statsForView.set invocation and the eligiblePersonsView lookup so
callers always receive a Set<string>.
- Around line 613-624: The function calculateWeightingFactor returns early with
no value when personScore is undefined or 0, violating its declared number
return type; change the early return to an explicit numeric default (e.g.,
return 1 as a neutral weighting) and ensure any callers expect a number; update
the check in calculateWeightingFactor to return 1 (or another agreed-upon
default numeric constant) when personScore is undefined or zero, and consider
adjusting the parameter type if undefined is valid.
- Around line 41-43: The null-check currently uses
settings.cong_settings.language_groups.enabled which can throw if
language_groups or cong_settings is undefined; update the conditional in
assignments_with_stats.ts that returns relevantViews to safely check the path
using optional chaining and the actual SettingsType shape (check
settings.cong_settings?.language_groups?.enabled?.value) so the guard reads the
boolean value without throwing; ensure you reference the same symbols: settings,
cong_settings, language_groups, enabled, value, and keep the early return of
relevantViews when the feature is not enabled.
In `@src/services/app/autofill.ts`:
- Around line 266-271: The code calls
WEEK_TYPE_ASSIGNMENT_PATH_KEYS.get(Week.CO_VISIT).has(key) without checking for
undefined which can throw; update the COWeekMain branch (where COWeekMain is
derived from mainWeekType === Week.CO_VISIT) to first retrieve the set via const
coSet = WEEK_TYPE_ASSIGNMENT_PATH_KEYS.get(Week.CO_VISIT) and guard against
undefined (e.g. if (!coSet) return relevantAssignmentKeys; or use coSet.has(key)
only when coSet exists), then filter relevantAssignmentKeys using coSet.has(key)
so .has is never called on undefined.
- Around line 729-762: The comparator passed to tasks.sort can return undefined
when all checks tie (diff === 0); update the comparator used in the return
tasks.sort(...) block to always return a number by adding an explicit final
return 0. Locate the comparator that references fixedAssignments,
linkedAssignments, AssignmentCode.MM_AssistantOnly, a.assignmentKey (e.g.,
'WM_Speaker_Part2'), a.dataView, and a.sortIndex and ensure after the diff check
you return 0 so equal items produce a deterministic sort order.
- Around line 95-104: meetingDay can be undefined if no record matches dataView,
which then gets passed to addDays; update the logic that computes meetingDay
(the ternary choosing from settings.cong_settings.midweek_meeting or
weekend_meeting using meeting_type, dataView) to provide a safe default (e.g., 0
or a specific weekday) when the find returns undefined before calling
addDays(weekOf, meetingDay), so addDays always receives a valid number.
- Around line 336-346: The function declares a return type of { code:
AssignmentCode | undefined; elderOnly: boolean } but currently uses bare
`return;` in the early-exit branches (after `const partMatch =
key.match(/AYFPart(\d+)/)` and after `const ayfSourceData =
source.midweek_meeting[\`ayf_part${partIndex}\`]`), which yields undefined;
replace those bare returns with an explicit object that matches the signature
(for example `return { code: undefined, elderOnly: false }`) so callers always
receive the expected shape from this function.
- Around line 949-956: The block checking AssignmentCode.MM_AssistantOnly
accesses studentPerson.person_uid without ensuring studentPerson is defined;
update the conditional to guard against undefined studentPerson (e.g., require
studentPerson truthiness before accessing its person_uid) or merge this logic
with the earlier studentPerson check used with isValidAssistantForStudent so you
never dereference studentPerson when it may be undefined; adjust the condition
that references task.code, studentPerson.person_uid, and p.person_uid
accordingly.
In `@src/services/app/persons.ts`:
- Around line 913-931: Rename the incorrectly named function hanldeIsPersonAway
to handleIsPersonAway and update all references/exports to that new identifier;
inside the function either keep the current logic or replace its body with a
call to the existing personIsAway(person, targetDate) to avoid duplication
(ensure you preserve return type boolean and imports/types for PersonType and
formatDate if you inline logic). Also search the codebase for any usages of
hanldeIsPersonAway and update them to handleIsPersonAway so consumers continue
to work.
🧹 Nitpick comments (9)
src/definition/assignment.ts (2)
35-40: Translate comments to English for consistency.The inline comments are in German. For codebase consistency and maintainability, please translate them to English.
✏️ Proposed translation
const getAssignmentCodes = (prefix: string): AssignmentCode[] => { return Object.values(AssignmentCode) - .filter((val) => typeof val === 'number') // Nur Zahlenwerte - .filter((val) => AssignmentCode[val as number].startsWith(prefix)) // Prüfen, ob der Key mit Prefix startet + .filter((val) => typeof val === 'number') // Only numeric values + .filter((val) => AssignmentCode[val as number].startsWith(prefix)) // Check if key starts with prefix .map((val) => val as AssignmentCode); };
3-4: Clarify deprecation status.The comments "Deprecated?" with question marks are ambiguous. If these values are deprecated, add proper JSDoc
@deprecatedannotations with migration guidance. If the deprecation status is uncertain, resolve it before merging.src/services/app/schedules.ts (3)
867-876: Remove commented-out code and translate comments.
- Remove the commented-out code block (lines 868-870) - it clutters the codebase
- Translate the German comment on line 867 to English
✏️ Proposed cleanup
- //ist der eingriff hier ein problem - /* if (assignment.includes('MM_Chairman')) { - history.assignment.code = AssignmentCode.MM_Chairman; - } */ + // Differentiate chairman codes: main hall vs auxiliary classroom if (assignment.includes('MM_Chairman_A')) { history.assignment.code = AssignmentCode.MM_Chairman; } if (assignment.includes('MM_Chairman_B')) { history.assignment.code = AssignmentCode.MM_AuxiliaryCounselor; }
2000-2009: Translate German comments to English.The comments explaining the dataView guard logic are in German. Please translate for codebase consistency.
✏️ Proposed translation
const dataView = store.get(userDataViewState); // remove record from history const previousIndex = history.findIndex( (record) => record.weekOf === schedule.weekOf && record.assignment.key === assignment && - // --- WICHTIGE ÄNDERUNG START --- - // Wir löschen nur, wenn der Eintrag auch zu unserem aktuellen View gehört! + // Only delete if entry belongs to current data view record.assignment.dataView === dataView - // --- WICHTIGE ÄNDERUNG ENDE --- );
2049-2058: Translate German comments to English.The comments for the optional dataView parameter are in German.
✏️ Proposed translation
export const schedulesAutofillSaveAssignment = ({ assignment, schedule, value, history, - dataView: dataViewOverride, // <--- NEU: Optionaler Parameter + dataView: dataViewOverride, // Optional parameter to override store value }: { schedule: SchedWeekType; assignment: AssignmentFieldType; value: PersonType; history: AssignmentHistoryType[]; - dataView?: string; // <--- NEU: Typ-Definition + dataView?: string; }) => { - // Wenn ein Override übergeben wurde, nimm den. Sonst hol aus dem Store. + // Use override if provided, otherwise get from store const dataView = dataViewOverride || store.get(userDataViewState);src/services/app/assignment_selection.ts (1)
534-541: Redundant DataView check in student task validation.The
entry.assignment.dataView === currentDataViewcheck at line 537 is redundant. At this point in the code,tasksInWeekonly contains entries that passed the dataView check (lines 504-508 returntrueearly for cross-dataView conflicts), so all remaining entries are guaranteed to be incurrentDataView.♻️ Proposed simplification
if (STUDENT_TASK_CODES.includes(currentTaskCode)) { const hasStudentPart = tasksInWeek.some( - (entry) => - entry.assignment.dataView === currentDataView && - STUDENT_TASK_CODES.includes(entry.assignment.code) + (entry) => STUDENT_TASK_CODES.includes(entry.assignment.code) ); if (hasStudentPart) return true; }src/services/app/autofill.ts (1)
764-815: Remove commented-out alternative implementation.This large block of commented-out code (alternative
getSortedTaskswith chronological ordering) should be removed before merging. If this approach might be needed in the future, consider documenting it in an ADR or issue rather than leaving it in the codebase.src/services/app/assignments_with_stats.ts (2)
592-592: Linter warning: forEach callback returns a value.The
Set.add()call returns the Set, which becomes the implicit return value of the arrow function. While this doesn't affect behavior (forEach ignores return values), wrapping in braces silences the linter.♻️ Proposed fix
- metrics.eligibleUIDS?.forEach((uid) => assignablePersonsSet.add(uid)); + metrics.eligibleUIDS?.forEach((uid) => { assignablePersonsSet.add(uid); });
182-190: Remove non-English comments.Several comments in this file are in German (e.g., "KORREKTUR", "Prüfung auf .size > 0", "Hinweis"). For consistency and maintainability in an English codebase, these should be translated.
src/features/meetings/week_range_selector/useWeekRangeSelector.tsx
Outdated
Show resolved
Hide resolved
|
@nobodyzero1: I just started playing with it a little bit, and I hope it’s not only me, but it seems like this introduces more repetition than what the current version does. Just tested with one month, and two persons get repeated twice in a month, although there are still a lot of available persons. |
|
Thanks @rhahao for testing this out! That is very interesting because I ran extensive simulations locally before submitting the PR and couldn't reproduce that repetition behavior. To verify the stability, I simulated a period of 1.5 years across three different data scenarios. I have attached the results of these simulations as Excel exports below. In these datasets, the distribution remains balanced, and I didn't observe the repetition issue. 📂 Simulations: Potential Cause: Long-Term Fairness vs. Short-Term Availability This might be the most likely explanation for what you are seeing. The algorithm distinguishes between available (free time) and prioritized (due for assignment). The only reason I can think of right now is the following: Could it be that the brothers receiving double assignments are those who are qualified for almost all types of tasks? And conversely, are the 'available' brothers only qualified for a limited number of task types? The first sorting criterion checks if the waiting time for any assignment is generally fulfilled (Global Tier). For brothers who can do everything, the math often dictates that they need an assignment almost every meeting on average. If they have also waited longer for this specific task than the others, they will be prioritized. Debugging the issue To find out exactly whether this is the intended "Catch-Up" logic or a bug, we need to inspect the scoring weights at the moment of assignment. Could you temporarily add to the end of sortCandidatesMultiLevel function the code from the file? |
Hi @rhahao ! Reason for Volatility: To fix this, I adjusted the sorting logic to prioritize time. Additionally, I capped the weighting factor at 1.5, as my tests showed this yields the most stable results. Test Results & Logic Constraints 📊I ran simulations with several theoretical congregation setups. The results show that tasks are now distributed very regularly. Testresult auto assignment.zip However, one specific goal could not be fully achieved: Ensuring that every person eventually receives every task type they are eligible for. After deep analysis, I concluded that this is mathematically nearly impossible under the current constraints, specifically regarding Elders (or "Generalists"):
Conclusion: In my opinion, the current solution is the correct approach, as users generally prefer not having multiple assignments in a single meeting.
Debugging & Verification Tools 🛠️To help verify these results, I have created a separate branch that includes additional debugging features (on top of the PR code): This branch provides:
Attached Analysis Tool: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/services/app/autofill.ts`:
- Around line 609-634: getTasksArray's signature declares meeting_type:
MeetingType but callers (e.g., handleDynamicAssignmentAutofill) pass MeetingType
| undefined; update getTasksArray to accept an optional/union type
(meeting_type?: MeetingType or meeting_type: MeetingType | undefined) so the
TypeScript types match the runtime check at the top of getTasksArray where it
tests !meeting_type, and adjust any internal/return type annotations if
necessary to satisfy strict mode — ensure the caller(s)
(handleDynamicAssignmentAutofill) keep passing the same variable without casting
and verify any other callers compile against the new signature.
- Around line 248-273: The CO_VISIT filtering logic must run whenever
mainWeekType === Week.CO_VISIT regardless of currentWeekTypeView; move the block
that computes coVisitKeys and filters relevantAssignmentKeys (currently
referencing WEEK_TYPE_ASSIGNMENT_PATH_KEYS, Week.CO_VISIT, coVisitKeys, and
relevantAssignmentKeys) out of the currentWeekTypeView !== Week.NORMAL
conditional and execute it after that conditional returns/assigns
relevantAssignmentKeys so language groups with Week.NORMAL still get CO_VISIT
syncing; ensure coVisitKeys is retrieved via
WEEK_TYPE_ASSIGNMENT_PATH_KEYS.get(Week.CO_VISIT) and applied with the same
null-safe filtering (coVisitKeys?.has(key) ?? false) so undefined maps block
keys as before.
🧹 Nitpick comments (3)
src/services/app/autofill.ts (3)
846-854:eligibilityMapViewparameter type should allowundefined.The parameter is typed
Map<AssignmentCode, Set<string>>(line 850), but the caller at line 1043 passes the result of.get(dataView)which can beundefined. The code handles it safely at line 854 via optional chaining, but the type signature is inaccurate.🔧 Proposed fix
const filterCandidates = ( persons: PersonType[], task: AssignmentTask, cleanHistory: AssignmentHistoryType[], - eligibilityMapView: Map<AssignmentCode, Set<string>>, + eligibilityMapView: Map<AssignmentCode, Set<string>> | undefined, checkAssignmentsSettingsResult: AssignmentSettingsResult ) => {
726-765:getSortedTasksmutates the input array.
Array.prototype.sort()sorts in place. While the current caller doesn't reuse the unsorted array, naming the functiongetSortedTaskssuggests it returns a new array. Consider using.toSorted()or spreading before sorting to prevent surprises for future callers.♻️ Proposed fix
- return tasks.sort((a, b) => { + return [...tasks].sort((a, b) => {
544-575:getForcedPerson: fixed assignments silently override linked assignments.If both a linked assignment and a fixed assignment match for the same task, the fixed person wins because it's checked second (line 570-573) and overwrites
forcedPerson. This is likely intentional (fixed = explicit override), but a brief comment would clarify the precedence.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/app/schedules.ts (1)
1990-2033: 🛠️ Refactor suggestion | 🟠 Major
schedulesAutofillUpdateHistoryignores thedataViewoverride fromschedulesAutofillSaveAssignment.
schedulesAutofillSaveAssignmentaccepts an optionaldataViewoverride (line 2047) and uses it when saving (line 2056), butschedulesAutofillUpdateHistory(called at line 2087) unconditionally readsstore.get(userDataViewState)(line 2001) instead of receiving the override. If anyone callsschedulesAutofillSaveAssignmentwith adataViewthat differs from the current store value, the history removal predicate (line 2007) and the new history entry would target the wrong view.Currently safe because the autofill caller derives its
dataViewfrom the same store, but the mismatch undermines the purpose of the override parameter.Proposed fix — thread dataView through to history update
export const schedulesAutofillUpdateHistory = ({ schedule, assignment, assigned, history, + dataView: dataViewOverride, }: { schedule: SchedWeekType; assignment: AssignmentFieldType; assigned: AssignmentCongregation; history: AssignmentHistoryType[]; + dataView?: string; }) => { - const dataView = store.get(userDataViewState); + const dataView = dataViewOverride || store.get(userDataViewState);And in
schedulesAutofillSaveAssignment:schedulesAutofillUpdateHistory({ schedule, assignment, assigned, history, + dataView, });Also applies to: 2042-2093
🤖 Fix all issues with AI agents
In `@src/services/app/autofill.ts`:
- Around line 845-923: filterCandidates currently returns the forcedPerson
immediately (forcedPerson from getForcedPerson) which skips availability and
conflict checks; instead validate forcedPerson with the same rules used for
other candidates (check allowedUIDs membership, personIsElder if task.elderOnly,
isPersonBlockedOnDate(task.targetDate), hasAssignmentConflict(...), and
assistant/student checks via isValidAssistantForStudent and MM_AssistantOnly
logic) and only return that person if all checks pass; if validation fails, fall
through to the normal filtering logic (or return an empty array) so you don't
create assignments for unavailable/conflicted people.
- Around line 69-108: The two functions getWeekType and getActualMeetingDate
currently accept meeting_type: MeetingType but callers pass MeetingType |
undefined, so add explicit handling: change both signatures to accept
meeting_type?: MeetingType (or MeetingType | undefined) and add a guard at the
top (e.g., if (meeting_type === undefined) { throw new Error("meeting_type is
required for getWeekType/getActualMeetingDate"); } ), then keep the existing
midweek vs weekend ternary logic unchanged; this prevents silent fall-through to
the weekend branch and makes the failure explicit for callers like
getTasksArray.
- Around line 1086-1092: After dbSchedBulkUpdate persists modifiedWeeks, update
the in-memory schedulesState before calling schedulesBuildHistoryList so the
history reflects the saved changes: ensure you either call the function that
merges modifiedWeeks into the schedulesState atom (or directly set
store.set(schedulesState, updatedSchedules)) and then call
schedulesBuildHistoryList(), finally store.set(assignmentsHistoryState,
newFullHistory). Update references: dbSchedBulkUpdate, schedulesState,
schedulesBuildHistoryList, and assignmentsHistoryState.
🧹 Nitpick comments (1)
src/services/app/autofill.ts (1)
463-522:getCodeAndElderOnly:keyparameter typed asstringbutAssignmentPathKeyexpected.The
keyparameter is typed asstring(line 464), but it's always called withAssignmentPathKeyvalues (line 663) and used withASSIGNMENT_DEFAULTS[key](line 514) which expectsAssignmentPathKey. Narrowing the type would catch misuse at compile time.
|
2 participants
Description
This Pull Request overhauls the automatic assignment logic to fix an issue where eligible persons were not being assigned automatically. It improves the automatic distribution of assignments. The primary goal is to ensure that everyone eligible for a specific task type gets assigned eventually. Simultaneously, the logic prioritizes assigning tasks to publishers with fewer qualifications more frequently compared to those eligible for a wide variety of tasks.
The logic is designed to be flexible enough to handle:
Logic Implementation Details:
1. Data Preparation
2. Task Creation
3. Statistics and Permissions
4. Task Sorting
5. Candidate Filtering and Assignment
6. Selection Strategy (
sortCandidatesMultiLevel)The best candidate is selected using a multi-level priority system:
Criterion 1: Recovery Tier Calculation
globalWaitTier(wait time relative to personal average).Criterion 2: Task-Specific Metrics
taskWaitTime: Weighted wait time since the last assignment of the same type.Criterion 3: Workload Evaluation
Fixes #4456
Type of change
Please delete options that are not relevant.
Checklist: