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

Allow setting label_ids on a message #231

Merged
merged 7 commits into from
Aug 22, 2019
Merged

Conversation

arunagw
Copy link
Contributor

@arunagw arunagw commented Aug 21, 2019

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.

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.
@adarsh adarsh self-requested a review August 21, 2019 18:59
Copy link
Contributor

@adarsh adarsh left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -46,6 +50,15 @@ def unread?
unread
end

def update(data)
unupdatable_attributes = data.keys.reject { |name| UPDATABLE_ATTRIBUTES.include?(name) }
Copy link
Contributor

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is beautiful ✨

@arunagw arunagw merged commit 0e18e7d into master Aug 22, 2019
@arunagw arunagw deleted the aa-add-label-ids-on-message branch August 22, 2019 16:42
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.

2 participants