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]: Notification setting logic updates #2752

Merged

Conversation

Ayush8923
Copy link
Contributor

@Ayush8923 Ayush8923 commented Dec 26, 2024

closes: #2723

Visible/Frontend Changes

  • Implement the logic to save the notification rules in CouchDB
  • Implement the logic that when the user enables or disables push notifications from the Where you receive notifications section in the notification settings page, the value is updated accordingly in the CouchDB.
  • Retrieve the existing notification rules from the CouchDB and then show all the notification rules.

Ayush8923 and others added 30 commits November 27, 2024 10:32
@Ayush8923 Ayush8923 requested a review from sleidig January 6, 2025 05:23
@Ayush8923 Ayush8923 requested a review from sleidig January 7, 2025 09:42
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

I did a fairly major further refactoring to simplify the layers of formControls and methods. Please have a look at the latest code and check if you find any errors or have further suggestions, @Ayush8923

@Ayush8923
Copy link
Contributor Author

Thank you, @sleidig, for refactoring the formControl. It looks much better now. However, there was a minor issue where, if the user had already chosen push as the notification method, it wasn't being auto-selected. To fix this, I made some final changes and removed the hardcoded values from the notification method. This way, in the future, if we need to add an email option to the notification methods, we can simply add a new key to the notification method without requiring any code-level changes.

I have checked the functionality, and it's working well. Could you please review the final changes once? If any further modifications are needed, please let me know. Otherwise, the PR is ready to merge.

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Thanks for testing and improving! Unfortunately I had to remove the notifcation-method-select component to make all the properties work with formControls easily but taking the options out into a variable is a good approach

@Ayush8923 Ayush8923 requested a review from sleidig January 8, 2025 08:04
@Ayush8923 Ayush8923 requested a review from sleidig January 8, 2025 12:24
@sleidig sleidig merged commit fdd6a31 into feat/1055_notifications-final Jan 8, 2025
11 of 12 checks passed
@sleidig sleidig deleted the feat/notification-setting-logic-updates branch January 8, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Notification Settings Logic for User Entity Updates in CouchDB
2 participants