-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
FEATURE: Mark all chat channels read with a shortcut #20629
Conversation
fc3c98d
to
f11c8ae
Compare
0554381
to
5417dc7
Compare
I fixed the conflicts following the move to zeitwerk autoloading @martin-brennan, just in case I took a patch file before doing the force-push: |
@jjaffeux I think this is good for another look now, changed the routes 👍 Though I haven't made any changes for the timestamp thing, I think we can do that separately? |
plugins/chat/config/routes.rb
Outdated
@@ -17,6 +17,8 @@ | |||
post "/channels/:channel_id/memberships/me" => "channels_current_user_membership#create" | |||
put "/channels/:channel_id/notifications-settings/me" => | |||
"channels_current_user_notifications_settings#update" | |||
put "/channels/read/" => "reads#update_all" |
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.
Sorry Im nitpicking here, but the _all
is not needed IMO
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.
But they both go to the same controller, are you saying they should still both be in the same endpoint with the switching based on params like I had before?
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 thinking two controllers... but you are right that might be weird, ok 👍
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.
Sweet, will leave as-is
@jjaffeux fixed the suggestions but just have one more question if you could please take a look |
This commit adds a keyboard shortcut (Shift+ESC) for chat which marks all of the chat channels that the user is currently a following member of as read, updating their `last_read_message_id`. This is done via a new service. It also includes some refactors and controller changes: * The old mark message read route from `ChatController` is now supplanted by the `Chat::Api::ReadsController#update` route. * The new controller can handle either marking a single or all messages read, and uses the correct service based on the route and params. * The `UpdateUserLastRead` service is now used (it wasn't before), and has been slightly updated to just use the guardian user ID.
This commit adds a keyboard shortcut (Shift+ESC) for chat which marks all
of the chat channels that the user is currently a following member of as read,
updating their
last_read_message_id
. This is done via a new service.It also includes some refactors and controller changes:
ChatController
is now supplantedby the
Chat::Api::ReadsController#update
route.and uses the correct service based on the route and params.
UpdateUserLastRead
service is now used (it wasn't before), and has been slightlyupdated to just use the guardian user ID.