Fix membership calculation (#2771)

This PR fixes two bugs that were causing students to be assigned a start year 1 year later than reality (e.g., 2024 instead of 2023).

### Fix loop overwrite in empty semesters
The logic for estimating progress during empty semesters (like bachelor semester 4) was flawed. We iterated through all previous semesters to guess the current year, but we were overwriting the result in every iteration. Now we `Math#max` the current and local values to find the highest instead.

### Fix off-by-one rounding error
We were converting semesters to years using `Math.floor(semester / 2)`. This failed for spring semesters.

> Example: semester 3 (spring of 2nd year) became `floor(1.5) = 1`. This made the system think 2nd-year students were only in their 1st year.

This is fixed by changing it to `Math.round(semester / 2)`, so that semester 3 correctly resolves to 2 years.

Author: brage-andreas

apps/rpc/src/modules/user/membership-service.ts

@@ -5,11 +5,11 @@ import type { NTNUGroup } from "../feide/feide-groups-repository"
 import { getLogger } from "@dotkomonline/logger"
 
 export interface MembershipService {
-  findApproximateMasterStartYear(courses: NTNUGroup[]): number
-  findApproximateBachelorStartYear(courses: NTNUGroup[]): number
+  findMasterStartYearDelta(courses: NTNUGroup[]): number
+  findBachelorStartYearDelta(courses: NTNUGroup[]): number
 }
 
-const BACHELOR_STUDY_PLAN_COURSES = [
+const BACHELOR_STUDY_PLAN = [
   {
     semester: 0,
     courses: ["IT2805", "MA0001", "TDT4109"],
@@ -28,7 +28,7 @@ const BACHELOR_STUDY_PLAN_COURSES = [
   },
   {
     semester: 4,
-    // NOTE: Semester 1 in Year 3 are all elective courses, so we do not use any of them to determine which year somebody
+    // Semester 1 in year 3 are all elective courses, so we do not use any of them to determine which year somebody
     // started studying
     courses: [],
   },
@@ -57,13 +57,13 @@ const MASTER_STUDY_PLAN = [
   },
 ] as const
 
-type StudyPlanCourseSet = typeof BACHELOR_STUDY_PLAN_COURSES | typeof MASTER_STUDY_PLAN
+type StudyPlanCourseSet = typeof BACHELOR_STUDY_PLAN | typeof MASTER_STUDY_PLAN
 
 export function getMembershipService(): MembershipService {
   const logger = getLogger("membership-service")
   // The study plan course set makes some assumptions for the approximation code to work as expected. In order to make
   // it easier for future dotkom developers, the invariants are checked here.
-  validateStudyPlanCourseSet(BACHELOR_STUDY_PLAN_COURSES)
+  validateStudyPlanCourseSet(BACHELOR_STUDY_PLAN)
   validateStudyPlanCourseSet(MASTER_STUDY_PLAN)
 
   function validateStudyPlanCourseSet(courseSet: StudyPlanCourseSet) {
@@ -143,7 +143,9 @@ export function getMembershipService(): MembershipService {
 
           // Take the mean distance for this semester
           const sum = previousSemesterDistances.reduce((acc, curr) => acc + curr, 0)
-          largestLocalSemester = Math.ceil(sum / previousSemesterDistances.length)
+          const currentSemesterEstimate = Math.ceil(sum / previousSemesterDistances.length)
+
+          largestLocalSemester = Math.max(largestLocalSemester, currentSemesterEstimate)
         }
 
         largestSemester = Math.max(largestSemester, largestLocalSemester)
@@ -167,15 +169,20 @@ export function getMembershipService(): MembershipService {
     }
 
     // Give the value back in years (two school semesters in a year).
-    return Math.floor(largestSemester / 2)
+    // We use Math#round because it will give us the correct year delta:
+    //   Year 1 fall (value 0)  : round(0 / 2) = 0 (Start year = current year)
+    //   Year 1 spring (value 1): round(1 / 2) = 1 (Start year = current - 1, since spring is in the next calendar year)
+    //   Year 2 fall (value 2)  : round(2 / 2) = 1 (Start year = current - 1)
+    //   Year 2 spring (value 3): round(3 / 2) = 2 (Start year = current - 2)
+    return Math.round(largestSemester / 2)
   }
   return {
-    findApproximateMasterStartYear(courses) {
+    findMasterStartYearDelta(courses) {
       return findApproximateStartYear(courses, MASTER_STUDY_PLAN)
     },
 
-    findApproximateBachelorStartYear(courses) {
-      return findApproximateStartYear(courses, BACHELOR_STUDY_PLAN_COURSES)
+    findBachelorStartYearDelta(courses) {
+      return findApproximateStartYear(courses, BACHELOR_STUDY_PLAN)
     },
   }
 }

apps/rpc/src/modules/user/membership.e2e-spec.ts

@@ -122,8 +122,8 @@ export function getUserService(
     // Master degree always takes precedence over bachelor every single time.
     const isMasterStudent = masterProgramme !== undefined
     const distanceFromStartInYears = isMasterStudent
-      ? membershipService.findApproximateMasterStartYear(courses)
-      : membershipService.findApproximateBachelorStartYear(courses)
+      ? membershipService.findMasterStartYearDelta(courses)
+      : membershipService.findBachelorStartYearDelta(courses)
     const estimatedStudyStart = subYears(getAcademicStart(getCurrentUTC()), distanceFromStartInYears)
 
     // NOTE: We grant memberships for at most one year at a time. If you are granted membership after new-years, you
@@ -215,7 +215,7 @@ export function getUserService(
     },
 
     async register(handle, userId) {
-      // NOTE: The registrer function here has a few responsibilities because of our data strategy:
+      // NOTE: The register function here has a few responsibilities because of our data strategy:
       //
       // 1. The database is the source of truth, and is ALWAYS intended to be as such.
       // 2. Unfortunately, there was a period in time where Auth0 was the source of truth, most notably right after we

apps/rpc/src/modules/user/user-service.ts

@@ -1,7 +1,7 @@
 import type { TZDate } from "@date-fns/tz"
 import { schemas } from "@dotkomonline/db/schemas"
 import { getCurrentUTC, slugify } from "@dotkomonline/utils"
-import { addYears, differenceInYears, isAfter, isBefore, setMonth, startOfMonth } from "date-fns"
+import { addYears, isAfter, isBefore, setMonth, startOfMonth } from "date-fns"
 import { z } from "zod"
 import { buildSearchFilter } from "./filters"
 
@@ -31,7 +31,7 @@ export type UserId = User["id"]
 export type UserProfileSlug = User["profileSlug"]
 
 export const NAME_REGEX = /^[\p{L}\p{M}\s'-]+$/u
-export const PHONE_REGEX = /^[0-9+-\s]*$/
+export const PHONE_REGEX = /^[0-9-+\s]*$/
 export const PROFILE_SLUG_REGEX = /^[a-z0-9-]+$/
 
 // These max and min values are arbitrary
@@ -93,25 +93,30 @@ export function findActiveMembership(user: User): Membership | null {
 }
 
 export function getMembershipGrade(membership: Membership): 1 | 2 | 3 | 4 | 5 | null {
-  // Take the difference, and add one because if `startYear == currentYear` they are in their first year
-  const delta = differenceInYears(getAcademicStart(getCurrentUTC()), getAcademicStart(membership.start)) + 1
+  const now = getCurrentUTC()
+
+  // Make sure we clamp the value to a minimum of 1
+  const delta = Math.max(1, getAcademicYearDelta(membership.start, now))
 
   switch (membership.type) {
     case "KNIGHT":
     case "PHD_STUDENT":
       return 5
+
     case "SOCIAL_MEMBER":
       return 1
+
     case "BACHELOR_STUDENT": {
       // Bachelor students are clamped at 1-3, regardless of how many years they used to take the degree.
-      return Math.max(1, Math.min(3, delta)) as 1 | 2 | 3
+      return Math.min(3, delta) as 1 | 2 | 3
     }
+
     case "MASTER_STUDENT": {
-      // Master students must be clamped at 4-5 because they can only be in their first or second year, but are always
-      // considered to have a bachelor's degree from beforehand.
-      const yearsGivenBachelors = delta + 3
-      return Math.max(4, Math.min(5, yearsGivenBachelors)) as 4 | 5
+      // Master students are clamped at 4-5, and are always considered to have a bachelor's degree from beforehand.
+      const yearsWithBachelors = delta + 3
+      return Math.min(5, yearsWithBachelors) as 4 | 5
     }
+
     case "OTHER":
       return null
   }
@@ -161,3 +166,25 @@ export function getNextAcademicStart(): TZDate {
   const isBeforeAugust = isBefore(now, firstAugust)
   return isBeforeAugust ? firstAugust : addYears(firstAugust, 1)
 }
+
+/**
+ * Calculates how many academic years have passed since the start date.
+ * If start is "last August" (current academic year), returns 1.
+ * If start was the August before that, returns 2.
+ */
+function getAcademicYearDelta(startDate: Date | TZDate, now: Date | TZDate = getCurrentUTC()): number {
+  const currentYear = now.getFullYear()
+  const currentMonth = now.getMonth() // 0-indexed (Jan=0, Aug=7)
+
+  // If we are in Jan-July (0-6), the academic year started in the PREVIOUS calendar year
+  // If we are in Aug-Dec (7-11), the academic year started in THIS calendar year
+  const academicYearCurrent = currentMonth >= 7 ? currentYear : currentYear - 1
+
+  // We do the same normalization for the membership start date
+  // (Handling cases where a member might join in Jan/Feb)
+  const startYear = startDate.getFullYear()
+  const startMonth = startDate.getMonth()
+  const academicYearStart = startMonth >= 7 ? startYear : startYear - 1
+
+  return academicYearCurrent - academicYearStart + 1
+}