-
Notifications
You must be signed in to change notification settings - Fork 651
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
Email notifications toggle API endpoint #15930
Email notifications toggle API endpoint #15930
Conversation
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.
LGTM, just left minor comments, nothing blocking 💪
private | ||
|
||
def valid_notification | ||
errors.add(:notification, 'Invalid notification') unless VALID_NOTIFICATIONS.include?(notification) |
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.
nitpicking: you can use the inclusion
validation helper: https://guides.rubyonrails.org/v4.2/active_record_validations.html#inclusion
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.
I'm having some issues applying this suggestion: in the spec file now I have to mock the constant instead of the valid_notification
method, and I can't figure out a good solution to achieve that
Thanks for the feedback thou :)
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.
then I guess you have two options: rely on rails doing the validation and not testing it, or keep it "as is" :)
(no big deal)
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.
I prefer to test it, even if the validation method is not the ideal, so I'm going to keep it as is
before_action :load_notifications, only: [:show, :update] | ||
|
||
rescue_from StandardError, with: :rescue_from_standard_error | ||
rescue_from Carto::LoadError, with: :rescue_from_carto_error |
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.
from my ignorance, why is this needed?
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.
Probably a leftover of a copy&paste from other controller, let me check and delete it
@@ -0,0 +1,41 @@ | |||
require_dependency 'carto/uuidhelper' |
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.
I think this is not needed
proc do | ||
create_table :user_email_notifications do | ||
Uuid :id, primary_key: true, default: Sequel.lit('uuid_generate_v4()') | ||
foreign_key :user_id, :users, type: :uuid, null: false, on_delete: :cascade |
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.
(I was wondering if this on_delete: :cascade
collides with user model dependent: :destroy
, but I see there's even a test for it 👍 )
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.
I'm leaving a couple of comments 👀
class EmailNotificationsController < ::Api::ApplicationController | ||
|
||
ssl_required :update | ||
before_action :load_notifications, only: [:show, :update] |
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.
I think the , only: [:show, :update]
is not needed, as for the time being this controller only has those two actions.
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.
You're right, it's useless
@@ -293,6 +294,28 @@ def dbdirect_effective_ip | |||
reload.dbdirect_ip | |||
end | |||
|
|||
# @param [Hash] notifications_hash A notification list with this format: {"notif_a" => true, "notif_b" => false} | |||
def email_notifications=(notifications_hash) |
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.
Keep in mind you're overriding the behavior of the email_notifications=
which Rails creates automatically when you declare the association:
has_many :email_notifications
Not sure if this might cause issues. The build
method might come in handy if you want to try it: https://apidock.com/rails/ActiveRecord/Associations/CollectionProxy/build
This PR enables two new API endpoints to enable/disable email notifications (such as DO notifications):
GET
/api/v3/email_notifications?api_key=abcd
Returns a JSON with the list of email notifications and the current status, for example:
PUT
/api/v3/email_notifications?api_key=abcd
Create or update a given list of notifications. This endpoint needs a JSON payload as request parameter, for example:
You don't need to always provide the complete list of notifications to update them.