-
Notifications
You must be signed in to change notification settings - Fork 137
Hahmed snooze conversation #384
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
Conversation
…com-ruby into hahmed-snooze-conversation
hahmed
left a comment
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
|
@hahmed Thanks! And no problem for making the changes. |
make suggested changes
ab95c2e to
78851df
Compare
| # Assign | ||
| intercom.conversations.assign(id: conversation.id, admin_id: '123', assignee_id: '124') | ||
|
|
||
| # Snooze |
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 you think it would be useful to clarify that it is epoch time here just in case people are confused with the time format? Also might be good to add one or two examples us snoozing for one hour or a day like you referenced in the test, i.e. nowtime + 5
| client.conversations.assign(id: '147', admin_id: '123', assignee_id: '124') | ||
| end | ||
|
|
||
| it 'snoozes a conversation' do |
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 a test to ensure it does not set time in the past and returns an 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.
@choran Is that necessary? We handle past timestamps with a specific error that raises a a BadRequest error with the SDK.
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.
sounds good 👍
|
|
||
| def snooze(reply_data) | ||
| snoozed_until = reply_data.fetch(:snoozed_until) { fail 'snoozed_until field is required' } | ||
| reply reply_data.merge(message_type: 'snoozed', type: 'admin') |
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.
HI @theandrewykim , sorry for the delay here, forgot to get back to this. I was going to merge this but I just had one further question ... should the snoozed_until value be passed in with the replay, similar to assignee_id in the assign? e.g.
reply reply_data.merge(message_type: 'snoozed', type: 'admin', snoozed_until: snoozed_until)
I could be missing something here, i was just comparing it to the assign. Let me know and I can merge it then
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 just checked this again and it looks like the assign is wrong, well not wrong but unnecessary, i.e. adding in the field in the merge after checking it exists in the fetch, so I think this is good
choran
left a comment
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.
👍
Edits to: #381