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

Notify users that the page has been updated and incite a reload #16130

Merged
merged 43 commits into from
Aug 26, 2024

Conversation

mrmir
Copy link
Contributor

@mrmir mrmir commented Jul 15, 2024

@mrmir mrmir changed the title Feature/meetings update flash Notify users that the page has been updated and incite a reload Jul 15, 2024
@oliverguenther oliverguenther force-pushed the feature/meetings-update-flash branch from 4c7d5c2 to 21f7178 Compare July 29, 2024 14:21
updated_at breaks when uploading attachments, as the same page will be
updated without us having a chance to update the header.

Instead, calculate an "etag" value from the meeting sections and agenda
items timestamps as well as the meeting's own lock_version.
@oliverguenther oliverguenther force-pushed the feature/meetings-update-flash branch from 21f7178 to f9252c1 Compare July 29, 2024 14:21
@oliverguenther oliverguenther force-pushed the feature/meetings-update-flash branch from 9191cbd to f5fc32e Compare August 12, 2024 14:03
Copy link
Contributor

@toy toy left a comment

Choose a reason for hiding this comment

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

Looks good overall, just few notes code wise, also when testing I noticed a problematic issue, as the flash is shown also following my own actions on the page, which will be very confusing, at least it does for adding/removing/renaming sections. I am thinking only of some way of section/agenda item actions to update the checksum in stimulus controller if it is still the same as one that we have.

Was Action cable considered, or is this use "too much" to start using Action cable in the project?

app/components/concerns/op_turbo/streamable.rb Outdated Show resolved Hide resolved
modules/meeting/app/models/meeting.rb Outdated Show resolved Hide resolved
frontend/src/turbo/flash-stream-action.ts Outdated Show resolved Hide resolved
@oliverguenther
Copy link
Member

Looks good overall, just few notes code wise, also when testing I noticed a problematic issue, as the flash is shown also following my own actions on the page, which will be very confusing, at least it does for adding/removing/renaming sections. I am thinking only of some way of section/agenda item actions to update the checksum in stimulus controller if it is still the same as one that we have.

We missed updating the meeting from the service update, which resulted in the incorrect header being updated. This appears to be fixed now, although we are aware with this approach of using dependent values that we'll likely miss more cases where the dialog accidentally triggers. That's one reason why it's still behind a feature flag for now.

Was Action cable considered, or is this use "too much" to start using Action cable in the project?

We want to introduce ActionCable as soon as we can use the PostgreSQL adapter. I believe that this is now possible with Turbo 8, but someone would have to take a look at the exact details.

@oliverguenther oliverguenther requested review from toy and removed request for dombesz August 16, 2024 07:27
@toy
Copy link
Contributor

toy commented Aug 16, 2024

Looks good overall, just few notes code wise, also when testing I noticed a problematic issue, as the flash is shown also following my own actions on the page, which will be very confusing, at least it does for adding/removing/renaming sections. I am thinking only of some way of section/agenda item actions to update the checksum in stimulus controller if it is still the same as one that we have.

We missed updating the meeting from the service update, which resulted in the incorrect header being updated. This appears to be fixed now, although we are aware with this approach of using dependent values that we'll likely miss more cases where the dialog accidentally triggers. That's one reason why it's still behind a feature flag for now.

I only understood now that the updating I mentioned was already there, done through turbo stream.

MeetingSectionsController has update_header_component_via_turbo_stream only in update and move, so create, destroy and drop still need it.

Also I noticed that MeetingAgendaItemsController#move calls move_item_within_section_via_turbo_stream and update_header_component_via_turbo_stream, but move_item_within_section_via_turbo_stream already includes call to update_header_component_via_turbo_stream.

@toy
Copy link
Contributor

toy commented Aug 16, 2024

There is still a way to miss changes due to polling which can be easily displayed by moving sections, and I also saw a problem with the state of sections.

  1. Create 3 sections (named a, b, c) and open it in two windows
  2. Next should be done quickly, in first window tell section a to move to bottom, and in the second window tell section b to move to bottom
  3. After poll first window gets the reload flash, but second does not

Problem with state is that in second window despite the order being a, c, b, the actions for moving are correctly updated, so section a shows actions to move to top and bottom, section c shows only actions to move to bottom and section b shows only actions to move to top.

@oliverguenther
Copy link
Member

@toy we address the remarks regarding the header. For the quick subsequent reorders, we opened an open point since it's inherent to the way section reordering works with acts_as_list (not sending the full order to the backend, but only updating parts of it).

Discussion for that continues in https://community.openproject.org/work_packages/57331

If you agree, we'd like to merge this in

@toy
Copy link
Contributor

toy commented Aug 20, 2024

I agree, it is anyway behind a feature flag. I commented on the way it is added when checking on it

@oliverguenther oliverguenther merged commit 7cbb9de into dev Aug 26, 2024
12 checks passed
@oliverguenther oliverguenther deleted the feature/meetings-update-flash branch August 26, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants