refactor(api): Use UpdatePreference use-case for all Subscriber Preference updates
#6889
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
UpdatePreference use-case for all Subscriber Preference updates
…criber-api-inbox-api
…criber-api-inbox-api
| const response = await updateGlobalPreferences({}, session); | ||
|
|
||
| expect(response.data.data.preference.channels).to.eql({}); | ||
| }); |
There was a problem hiding this comment.
This test case asserts that empty objects can be supplied when updating global preferences in relation to this UpsertPreferences change
There was a problem hiding this comment.
As you will see further down, it's not ideal that we have some mapping logic in the controller level. I decided that's a suitable approach for now as we'll be performing a Subscriber API overhaul soon.
There was a problem hiding this comment.
A bunch of deletions for the old use-cases that only updated V1 Preferences 🧹
| @@ -67,6 +66,7 @@ export class UpdatePreferences { | |||
| return await this.findPreference(command, subscriber); | |||
| } | |||
|
|
|||
| @Instrument() | |||
There was a problem hiding this comment.
Adding some more instrumentation to the update preference flow.
There was a problem hiding this comment.
In a follow-up PR (to keep this diff clean), I will rename this usecase from UpdatePreferences to UpsertSubscriberPreferences as it functions as such now.
| } else { | ||
| await this.updateUserPreference(command, subscriber); | ||
| } | ||
| await this.updateSubscriberPreference(command, subscriber); |
There was a problem hiding this comment.
As said above, we treat this as an upsert now. GetSubscriberPreferences always returns a valid preference set now (falling back to default when nothing has been persisted), so there's no need to perform an extra lookup now before persisting the data.
…criber-api-inbox-api
…criber-api-inbox-api
…criber-api-inbox-api
…criber-api-inbox-api
| } else { | ||
| await this.updateUserPreference(command, subscriber); | ||
| } | ||
| await this.updateSubscriberPreference(command, subscriber); |
| }); | ||
| ): Promise<Omit<UpdateSubscriberPreferenceResponseDto, 'template'>> { | ||
| const channels = body.preferences?.reduce((acc, curr) => { | ||
| acc[curr.type] = curr.enabled; |
There was a problem hiding this comment.
Is this enabled flag the same as the one we talked about in skipped tests?
There was a problem hiding this comment.
No, this is the channel-level flag. There are 2 enabled flags:
enabledat top level of V1 Preferences, which controls whether to use the subscriber preference during preference mergingenabledflag for each channel, which controls delivery for the channel
What changed? Why was the change needed?
UpdatePreferenceuse-case for all Subscriber Preference updatesenabledflagenabledflag was introduced at a time beforecriticalworkflows were available. Setting theenabledflag to false on a preference set previously had the effect of preventing that Subscriber's preference settings from being read during notification delivery preference resolution. The Workflowcriticalflag handles this use-case in a more read-optimal way by requiring only a single Workflow resource to be mutated, rather than all Subscriber resources. I have left these tests in the codebase for now, we can debate further on complete deletion of theenabledflag as part ofNV-4597UpdatePreferenceuse-caseScreenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer