fix: late enrollment should be less sensitive to courserun state

[pwnage101]

ENT-9259

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.03%. Comparing base (6d1860b) to head (b6982d7).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1130      +/-   ##
==========================================
+ Coverage   87.00%   87.03%   +0.03%     
==========================================
  Files         388      388              
  Lines        8066     8070       +4     
  Branches     1939     1979      +40     
==========================================
+ Hits         7018     7024       +6     
+ Misses        999      997       -2     
  Partials       49       49              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[brobro10000]

Just 2 nits, great job! Approved to unblock 👍🏽

[brobro10000]

[nit] I believe ? after courseRun should achieve the same results as !('seats' in courseRun)

const baseNonAvailalbeCheck = [
    isArchived(courseRun),
    !courseRun?.seats?.length,
    !courseRun?.marketingUrl,
]

[brobro10000]

[nit/question] is there any chance of isEnrollableBufferDays to return as a null? Could !isEnrollableBufferDays achieve the same results?

[adamstankiewicz]

I was thinking along the same lines, @brobro10000. But !isEnrollableBufferDays could, in theory, be 0 would be truthy with the check on !isEnrollableBufferDays (when it is defined, it returns the value of a constant 30 but what if it were to change to 0 for some reason, or otherwise be configurable/dynamic).

Maybe checking for whether the value is a number? The (below) latter !isNaN check is because typeof NaN === 'number' is also true.

typeof isEnrollableBufferDays === 'number' && !isNaN(isEnrollableBufferDays) ? lateEnrollmentAvailableCourseRunsFilter : standardAvailableCourseRunsFilter,

Another alternative suggestion might be use the util functions in this repo like isDefined, isDefinedAndNotNull, etc.

isDefinedAndNotNull(isEnrollableBufferDays) ? lateEnrollmentAvailableCourseRunsFilter : standardAvailableCourseRunsFilter,

[unrelated nit / follow-up] I wonder if we might want to consider eventually renaming isEnrollableBufferDays since it seems to indicate its value is a boolean due to the is prefix, but it returns an integer constant or isn't defined. Perhaps something like lateEnrollmentBufferDays would make sense.

[adamstankiewicz]

LGTM, with 2 small nits/suggestions to consider :)

[adamstankiewicz]

[nit/suggestion]: Perhaps updated slightly / simplified a bit:

const baseNonAvailableChecks = [
  isArchived(courseRun),
  !courseRun.seats?.length,
  // ...
];

The check ('seats' in courseRun) is handled by the optional chaining of ?. in courseRun.seats?.length. I don't think there needs to be ?. in courseRun?.seats, though, since other existing code paths assume courseRun exists (e.g., courseRun.isMarketable).

[adamstankiewicz]

(Similar to Hamzah's comment below)

[adamstankiewicz]

I was thinking along the same lines, @brobro10000. But !isEnrollableBufferDays could, in theory, be 0 would be truthy with the check on !isEnrollableBufferDays (when it is defined, it returns the value of a constant 30 but what if it were to change to 0 for some reason, or otherwise be configurable/dynamic).

Maybe checking for whether the value is a number? The (below) latter !isNaN check is because typeof NaN === 'number' is also true.

typeof isEnrollableBufferDays === 'number' && !isNaN(isEnrollableBufferDays) ? lateEnrollmentAvailableCourseRunsFilter : standardAvailableCourseRunsFilter,

Another alternative suggestion might be use the util functions in this repo like isDefined, isDefinedAndNotNull, etc.

isDefinedAndNotNull(isEnrollableBufferDays) ? lateEnrollmentAvailableCourseRunsFilter : standardAvailableCourseRunsFilter,

[unrelated nit / follow-up] I wonder if we might want to consider eventually renaming isEnrollableBufferDays since it seems to indicate its value is a boolean due to the is prefix, but it returns an integer constant or isn't defined. Perhaps something like lateEnrollmentBufferDays would make sense.