-
Notifications
You must be signed in to change notification settings - Fork 130
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
Allow setting label_ids
on a message
#231
Conversation
Right now there is no way to set a `label` on a `message`. API accepts `label_ids` as an array of string of label ids. This PR also add `UPDATABLE_ATTRIBUTES` to just allow few attributes to be updated allowed by API instead of having all attributes to be updated.
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.
A few minor questions. Looks good to me.
has_n_of_attribute :labels, :label | ||
has_n_of_attribute :label_ids, :string |
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.
Do we need to add starred
and unread
attributes here also? I'm not familiar with the DSL so I'm just looking at the pattern.
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 am going to check with these attributes and see what's happening. If we don't need it I will remove them from here. Thanks for the point.
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.
We do have started
and unread
define above the file so we are good. The label_ids
was the missing one.
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.
lib/nylas/message.rb
Outdated
@@ -46,6 +50,15 @@ def unread? | |||
unread | |||
end | |||
|
|||
def update(data) | |||
unupdatable_attributes = data.keys.reject { |name| UPDATABLE_ATTRIBUTES.include?(name) } |
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.
What do you think about extracting a private method, something named like def unupdatable_attributes?
to raise the exception. Then it would maybe look like
def update(data)
if unupdatable_attributes?(data)
raise ArgumentError, # message
else
super(data)
end
end
?
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.
Make sense.
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 have added a new class to filter attributes here e40e7ce
I think we are going to need this class everywhere where we don't want to send unwanted params to server.
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.
Oh this is beautiful ✨
Right now there is no way to set a
label
on amessage
.API accepts
label_ids
as an array of string of label ids.This PR also add
UPDATABLE_ATTRIBUTES
to just allow few attributes tobe updated allowed by API instead of having all attributes to be
updated.