Move speech document #4573
Open
Move speech document #4573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sairamya93
requested review from
KludgeKML,
leenagupte,
deborahchua and
georges1996
KludgeKML
reviewed
Jan 9, 2025
sairamya93
force-pushed
the
move-speech-document
branch
from
876a571 to
01f9f7e
Compare
sairamya93
force-pushed
the
move-speech-document
branch
from
01f9f7e to
ce2b218
Compare
sairamya93
force-pushed
the
move-speech-document
branch
from
ce2b218 to
c08cd5b
Compare
sairamya93
requested review from
KludgeKML,
deborahchua,
georges1996 and
leenagupte
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
deborahchua
reviewed
Jan 10, 2025
sairamya93
force-pushed
the
move-speech-document
branch
from
cd61575 to
80036bf
Compare
leenagupte
reviewed
Feb 12, 2025
config/locales/ar.yml
Outdated
There was a problem hiding this comment.
I didn't not realise that the document_type and schema for a speech don't necessarily map: https://github.com/alphagov/publishing-api/blob/main/content_schemas/dist/formats/speech/frontend/schema.json#L34-L42
It also explains the need for the oral_statement key. 🤯
I think it's worth explaining this in the commit message.
config/locales/ar.yml
Outdated
There was a problem hiding this comment.
I think authored_article need to be added too: https://github.com/alphagov/government-frontend/blob/main/config/locales/en.yml#L101-L103
sairamya93
force-pushed
the
move-speech-document
branch
from
80036bf to
62f0305
Compare
sairamya93
force-pushed
the
move-speech-document
branch
2 times, most recently
from
396e550 to
34a015e
Compare
sairamya93
force-pushed
the
move-speech-document
branch
from
34a015e to
e86cb9d
Compare
leenagupte
previously approved these changes
Feb 20, 2025
leenagupte
dismissed
their stale review
Sorry Ramya! I forgot to mention that you need to add emphasised organisations as one of the contributors.
As you can see from the speech schema it is possible to have emphasised organisations for a speech, and the old linkable presenter would have added them if they exist, so they need to be added here too.
sairamya93
force-pushed
the
move-speech-document
branch
from
e86cb9d to
d8b49f8
Compare
- Added Speech model that includes the related concerns for Speech along with the tests - Modified Speech model to include the methods to retrieve the important-metadata information that were copied over from Speech Presenter in government-frontend - Added Speech Presenter and tests to handle the delivery_type of the speech Commit audit trail - https://github.com/alphagov/government-frontend/blob/122ae292fa540059ee165a94f8129d873c5205d5/app/presenters/speech_presenter.rb Audit trail for presenter tests - https://github.com/alphagov/government-frontend/blob/122ae292fa540059ee165a94f8129d873c5205d5/test/presenters/speech_presenter_test.rb
- Commit audit trail: https://github.com/alphagov/government-frontend/blob/122ae292fa540059ee165a94f8129d873c5205d5/app/presenters/content_item/news_image.rb - Commit audit trail for tests: https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/test/presenters/content_item/news_image_test.rb
- Commit audit trail: https://github.com/alphagov/government-frontend/blob/e7b9db33536f487991ba907502080e0625a066cc/app/presenters/content_item/political.rb - Added political tests and included the possibility of having political being false even though it is not likely to be. Reference doc: https://github.com/alphagov/whitehall/blob/13763a5b148ba06cb0e5be4139040dae311fa781/docs/history_mode.md#L11
As there is a possibility to have people but no speaker, the people concern is included in Speech model. Audit trail: https://github.com/alphagov/government-frontend/blob/6285583f2385567f785992fd7901209d8ddb7980/app/presenters/content_item/linkable.rb#L4
- Modified any_updates? method with guard clause - Added a missing test in Updatable concern Add missing test for updatable concern
- To handle the translations in history_notice partial - Audit trail: https://github.com/alphagov/government-frontend/tree/90cf5386b165b3a342689744feece7c83449b153/config/locales
Copied over the View from government-frontend and made appropriate changes Commit audit trail - https://github.com/alphagov/government-frontend/blob/3c8d12788047ec9ebd5c7f044f4e3f3fce0480ca/app/views/content_items/speech.html.erb - Updated the context and context_locale to reflect the correct translations - Modified code to render publisher_metadata - Modified withdrawn notice code in view - Modified code to render important-metadata - Audit trail for important-metadata change: alphagov/government-frontend@b1da197 - Updated the component with /heading appropriate parameters instead of title component - Removed the check for important_metadata - Modified code to have important-metadata directly in View instead of in the presenter
- All the locale keys for Speech are copied over from government-frontend - Ran the command : rake "consolidation:copy_translation[speech,formats.speech]" Audit trail: https://github.com/alphagov/government-frontend/blob/ed44dc0a2dc1cc14174f99bdf6f1ed57f2bfc77a/config/locales
- Ran the command: consolidation:copy_translation[content_item.schema_name.written_statement,formats.written_statement] Audit trail: https://github.com/alphagov/government-frontend/tree/ed44dc0a2dc1cc14174f99bdf6f1ed57f2bfc77a/config/locales
- To handle the pluralization for Speech in all the locales Run the command: rake "consolidation:copy_translation[content_item.schema_name.speech,formats.speech]" Commit audit trail: https://github.com/alphagov/government-frontend/tree/122ae292fa540059ee165a94f8129d873c5205d5/config/locales
- Ran the command: rake "consolidation:copy_translation[content_item.schema_name.oral_statement,formats.oral_statement]" Audit trail: https://github.com/alphagov/government-frontend/tree/ed44dc0a2dc1cc14174f99bdf6f1ed57f2bfc77a/config/locales
- Added request tests and modified the system tests to rpsec tests in frontend - Commit audit trail: https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/test/integration/speech_test.rb
Ran the command : govuk-docker-run rake "consolidation:copy_translation[content_item.schema_name.authored_article,formats.authored_article]" Audit trail: https://github.com/alphagov/government-frontend/blob/8799336fd3c96dac7c7e53cce3cd8a522beb17da/config/locales/en.yml#L101-L103 Speech document type comprises of all the below: - speech - authored_article - written_statement - oral_statement Reference document: https://github.com/alphagov/publishing-api/blob/648bddda06ad1afaeeebe2dc5f6f97da214d14ef/content_schemas/dist/formats/speech/frontend/schema.json#L34-L42
sairamya93
force-pushed
the
move-speech-document
branch
from
d8b49f8 to
0d9fe8c
Compare
leenagupte
reviewed
Feb 26, 2025
| include Political | ||
| include Updatable | ||
|
|
||
| def contributors | ||
| (organisations_ordered_by_emphasis + [speaker].flatten + [speaker_without_profile].flatten).compact | ||
| (organisations_ordered_by_emphasis + people + [speaker].flatten + [speaker_without_profile].flatten).compact.uniq { |contributor| contributor["content_id"] } |
There was a problem hiding this comment.
`{ |contributor| contributor["content_id"] }
What is this block supposed to do?
There was a problem hiding this comment.
Is speaker or speaker_without_profile an array? If they are, then they don't need to be made into an array by wrapping them in []. If they aren't arrays, then flatten isn't required because the array will only contain one item.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
To handle the rendering of Speech document type from government_frontend to frontend as part of [RFC 175]
Why
https://trello.com/c/IKDY5W9u/371-move-document-type-speech-from-government-frontend-to-frontend
How
To add a scoped route for, a controller, a model and a view. Here we split up the methods in the original file into presenter, model and its related concerns, with the minimum functionality required to get Speech document working.
Screenshots?
Frontend
Government-Frontend
Metadata