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

Maintain lead organisations first when publishing HTML attachments #9166

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

KludgeKML
Copy link
Contributor

@KludgeKML KludgeKML commented Jun 17, 2024

Currently HTML Attachments are publishing with organisations in default order (which means that there's no way for frontend apps to order by putting lead organisations first in the list). This is in contrast to the attachables, which seem to retain some ordering where lead organisations are listed first, example the two links in this ticket:

https://govuk.zendesk.com/agent/tickets/5448531

https://www.gov.uk/government/consultations/improving-the-experiences-of-people-with-mecfs-interim-delivery-plan
https://www.gov.uk/government/consultations/improving-the-experiences-of-people-with-mecfs-interim-delivery-plan/consultation-document-the-interim-delivery-plan-on-mecfs

https://trello.com/c/7d3ERApJ/23-lead-organisation-logos-in-html-publications

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@KludgeKML KludgeKML requested a review from ryanb-gds June 17, 2024 16:06
@KludgeKML KludgeKML force-pushed the html-attachment-org-ordering branch from 0145dc7 to 3f7a05f Compare June 18, 2024 09:52
@KludgeKML KludgeKML marked this pull request as ready for review July 9, 2024 14:23
@@ -38,7 +38,7 @@ def links
def edition_links
{
parent: parent_content_ids, # please use the breadcrumb component when migrating document_type to government-frontend
organisations: parent.organisations.pluck(:content_id).uniq,
organisations: (parent.lead_organisations.pluck(:content_id) + parent.supporting_organisations.pluck(:content_id)).uniq,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's already an existing method for retrieving the lead_org_id (and it uses .try to fail gracefully). Can you reuse that method, and add an equivalent for supporting_org_ids that uses .try in the same way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, just spotted the existing method only returns one lead org ID, you probably want them all. But lead_org_ids seems a fair method to add.

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've added in lead_org_ids and supporting_org_ids methods to tidy this up while giving the code a bit more robustness.

@@ -20,6 +20,10 @@ def access_limited_object

delegate :organisations, to: :parent_attachable

delegate :lead_organisations, to: :parent_attachable

delegate :supporting_organisations, to: :parent_attachable
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test that can be added to ensure CallForEvidenceResponse and ConsultationResponse delegate these methods? As removing these lines would presumably cause some issues down the line!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Have added tests for this.

@KludgeKML KludgeKML force-pushed the html-attachment-org-ordering branch 2 times, most recently from 02d8619 to 521daaa Compare July 22, 2024 10:00
- add test for ordering.
- update existing test to handle new mock requirements
- add delegation for call_for_evidence and consultation_response so
  that they can retrieve lead and supporting organisations
  separately to pass to presenter.
- add tests for delegated methods
@KludgeKML KludgeKML force-pushed the html-attachment-org-ordering branch from 521daaa to 921c6c4 Compare July 22, 2024 10:04
@KludgeKML KludgeKML enabled auto-merge July 22, 2024 10:06
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks

@KludgeKML KludgeKML merged commit 7468d4c into main Jul 22, 2024
19 checks passed
@KludgeKML KludgeKML deleted the html-attachment-org-ordering branch July 22, 2024 10:09
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