Skip to content
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

feat(api): Delete subscriber channel preference when updating global channel #6767

Merged

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Oct 24, 2024

What changed? Why was the change needed?

  • API
    • Delete subscriber channel preference when updating global channel
      • This ensures that the global preference can take priority again after updating the global preference
    • Return only the active & non-critical channels for Global Subscriber preferences
      • Previously all channels were shown regardless of the active or critical state of the Workflow. This change ensures the Subscriber only sees channels relevant to them, as all the other channels are not relevant and may cause confusion
    • Persist only the provided values for all preference sets with the exception of Workflow Resource preferences, to ensure that PATCH operations for preferences don't create preferences for non-relevant channels
    • Refactoring
      • Rename GetPreferences for Inbox to GetInboxPreferences to eliminate naming duplication with the generic GetPreferences use-case
      • Refactor preference use-cases to eliminate duplication and ease transition to V2 Preferences
    • TODO: should we throw a 400 Bad Request response when attempting to modify global preference channels that don't have active & non-critical workflows that would be affected? Right now, the PATCH responses for global preferences return undefined for channels that are irrelevant and it doesn't make much sense. We have failing tests that need to be addressed with the outcome of this decision.
  • <Inbox />
    • Optimistically update Subscriber Workflow preferences in <Inbox /> when modifying the Subscriber Global preferences, to ensure that the unset Workflow preferences display immediately.

Screenshots

Toggling the Subscriber Global Channel Preferences in <Inbox /> modifies the corresponding Subscriber Workflow Channel Preference fields.
Important Note: This PR only has changes to reflect local state changes optimistically, it does not refetch all Preferences from the API after the optimistic update.

global-preference-unset-workflow-pref.mov

<Inbox /> displays only Global Preference channels for active & non-critical workflow channels
image

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing defaults to ensure that only the provided mutation values are persisted, enabling PATCH operations to work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

netlify bot commented Oct 24, 2024

Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name Link
🔨 Latest commit 2137bc3
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/671f4929be35d800080923ae
😎 Deploy Preview https://deploy-preview-6767--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -323,6 +329,10 @@ export class Sync {
return notificationGroupId;
}

private getWorkflowPreferences(workflow: DiscoverWorkflowOutput): WorkflowPreferencesPartial {
return workflow.preferences || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fallback empty object is now necessary during sync after strengthening validation of the UpsertWorkflow use-case to disallow undefined preferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in the following files, we'll see renaming from GetPreferences to GetInboxPreferences to eliminate the duplicated usage of GetPreferences use-case naming.

Copy link
Contributor Author

@rifont rifont Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slightly bad diff, as the file is not entirely new. This use-case is however now considerably simpler, owing to better encapsulation of both:

  • GetSubscriberGlobalPreference filtering out channels in the response to include only active & non-critical channels
  • GetSubscriberPreference filtering out preference sets to include only active & non-critical workflows
  • Full encapsulation of V1 & V2 preferences in both of the above use-cases

Copy link
Contributor Author

@rifont rifont Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bad diff - it has an extra test spec which caused it to become bad after renaming from get-preferences -> get-inbox-preferences

Copy link

pkg-pr-new bot commented Oct 28, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/novuhq/novu/@novu/js@6767
pnpm add https://pkg.pr.new/novuhq/novu/@novu/nextjs@6767
pnpm add https://pkg.pr.new/novuhq/novu/@novu/react@6767
pnpm add https://pkg.pr.new/novuhq/novu/@novu/react-native@6767

commit: 2137bc3

with:
mongodb-version: 4.2.8
mongodb-version: 5.0.29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return this.callWithSession(async () =>
updatePreference({ emitter: this._emitter, apiService: this._inboxService, args })
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update method on the full Preferences resource was never used, so I'm deleting it.

@rifont rifont merged commit bd2f04d into next Oct 28, 2024
39 checks passed
@rifont rifont deleted the nv-4498-forget-workflow-subscriber-preferences-if-global-channel branch October 28, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants