Skip to content

[#70336] Sections in meeting agenda not displayed correctly when moved #21595

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.

Sign up for GitHub

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

Merged
mrmir merged 3 commits into from
Jan 9, 2026
Merged

[#70336] Sections in meeting agenda not displayed correctly when moved #21595

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
678de9c
Use preload instead of includes for sections
mrmir Jan 8, 2026
4b78f2b
Add spec
mrmir Jan 8, 2026
f7c342d
Refactor spec
mrmir Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion modules/meeting/app/controllers/meetings_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,8 @@ def find_meeting

@meeting = scope
.visible
.includes([:project, :author, { participants: :user }, :sections, { agenda_items: :outcomes }])
.includes([:project, :author, { participants: :user }, { agenda_items: :outcomes }])
.preload(:sections)
.find(params[:id])
end

Expand Down
38 changes: 38 additions & 0 deletions modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,5 +513,43 @@
show_page.expect_blankslate
end
end

it "maintains section order when rendering" do
section1 = create(:meeting_section, meeting:, title: "Section A")
section2 = create(:meeting_section, meeting:, title: "Section B")
section3 = create(:meeting_section, meeting:, title: "Section C")

show_page.visit!
show_page.expect_section(title: "Section A")
show_page.expect_section(title: "Section B")
show_page.expect_section(title: "Section C")

initial_positions = [section1, section2, section3].map(&:position)
expect(initial_positions).to eq([1, 2, 3])

show_page.select_section_action(section3, "Move up")

wait_for_network_idle

expect([section1, section2, section3].map { |s| s.reload.position }).to eq([1, 3, 2])

sections = page.all(".op-meeting-section-container").select do |section|
section["data-test-selector"]&.start_with?("meeting-section-container-") &&
!section["data-test-selector"]&.include?("header")
end
Copy link
Member

@cbliard cbliard Jan 8, 2026

Choose a reason for hiding this comment

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

As it's used twice in the test, I would put this into show_page as an helper.

Also, I think it can select only the header element, so it's start_with?("meeting-section-header-container-")

And the condition can be expressed using css with [attr^='value'] which means "starts with".

it becomes:

sections = page.all(".op-meeting-section-container[data-test-selector^='meeting-section-header-container-']")

once you have extracted this into a #section_headers, you can then write the assertions like this:

expect(show_page.section_headers.map(&:text))
  .to eq(["Section A",
          "Section C",
          "Section B"])

Copy link
Member

@cbliard cbliard Jan 8, 2026

Choose a reason for hiding this comment

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

It may also be better to have show_page.section_headers return text directly. Actually I think anything returning capybara object should be suffixed with element, so if it returns the capybara objects, it should be show_page.section_header_elements.

Yep, probably better to have show_page.section_headers return text directly. If someone needs the elements at some point, they can refactor.

Copy link
Contributor Author

@mrmir mrmir Jan 9, 2026

Choose a reason for hiding this comment

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

Done, thanks for the feedback 👍🏽

expect(sections[0]).to have_content("Section A")
expect(sections[1]).to have_content("Section C")
expect(sections[2]).to have_content("Section B")

show_page.visit!

sections_after_refresh = page.all(".op-meeting-section-container").select do |section|
section["data-test-selector"]&.start_with?("meeting-section-container-") &&
!section["data-test-selector"]&.include?("header")
end
expect(sections_after_refresh[0]).to have_content("Section A")
expect(sections_after_refresh[1]).to have_content("Section C")
expect(sections_after_refresh[2]).to have_content("Section B")
end
end
end
Loading