-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(api-service,worker): context aware schedule fixes NV-7064 #9865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
WalkthroughThis pull request implements context-aware scheduling for subscribers throughout the system. Changes include adding a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/api/src/app/events/e2e/trigger-event.e2e.ts`:
- Around line 3795-3808: The afterEach restores feature flags by assigning
stored values back to process.env which will set the literal string "undefined"
if the original was undefined; instead, check the saved variables
isSubscribersScheduleEnabled and isContextPreferencesEnabled and if a saved
value is undefined use delete (remove the env key) rather than assigning the
string, otherwise restore the saved string value to process.env—apply this logic
to the restoration lines that reference IS_SUBSCRIBERS_SCHEDULE_ENABLED and
IS_CONTEXT_PREFERENCES_ENABLED.
- Around line 4512-4710: The test fails due to invalid 12‑hour time strings
produced by createScheduleIncludingCurrentTime (e.g., "13:00 PM" or "00:00 AM");
update createScheduleIncludingCurrentTime to always output properly normalized
12‑hour times ("hh:mm AM/PM") by converting a 24h hour to (hour % 12 === 0 ? 12
: hour % 12), zero‑padding minutes, and selecting AM/PM based on the original
hour (hour < 12 -> AM else PM); ensure any place building weeklySchedule entries
uses this formatter so tests no longer produce invalid times and remain stable.
In `@apps/api/src/app/inbox/e2e/session.e2e.ts`:
- Around line 1172-1245: The test "should return context-specific schedule when
multiple contexts exist" mutates process.env.IS_CONTEXT_PREFERENCES_ENABLED
without guaranteeing cleanup; capture the original value before setting it, wrap
the test body that sets (process.env as any).IS_CONTEXT_PREFERENCES_ENABLED =
'true' and subsequent assertions in a try/finally, and in finally restore
process.env.IS_CONTEXT_PREFERENCES_ENABLED back to the original value (or delete
it if it was undefined) so the flag cannot leak to other tests; update the test
function surrounding the setup/teardown to use this pattern.
In `@apps/worker/src/.env.test`:
- Around line 98-100: Reorder the three environment keys so they follow
alphabetical order required by dotenv-linter: move
IS_CONTEXT_PREFERENCES_ENABLED to appear before IS_PUSH_UNREAD_COUNT_ENABLED,
keeping IS_SUBSCRIBERS_SCHEDULE_ENABLED as the first line; update the block
containing IS_SUBSCRIBERS_SCHEDULE_ENABLED, IS_CONTEXT_PREFERENCES_ENABLED, and
IS_PUSH_UNREAD_COUNT_ENABLED accordingly to eliminate the UnorderedKey warning.
What changed? Why was the change needed?
This pull request primarily adds new end-to-end tests for context-aware subscriber scheduling in the event trigger flow, and refactors test setup code for improved readability and maintainability. The most significant changes are grouped below:
New context-aware schedule tests for event triggers:
IS_SUBSCRIBERS_SCHEDULE_ENABLEDand the newIS_CONTEXT_PREFERENCES_ENABLEDfeature flags for the schedule logic tests.