Move speech document #4573
Move speech document #4573
Conversation
sairamya93
requested review from
KludgeKML,
leenagupte,
deborahchua and
georges1996
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
sairamya93
force-pushed
the
move-speech-document
branch
3 times, most recently
from
a50a329 to
cd61575
Compare
| @@ -0,0 +1,4 @@ | |||
| .inverse-background { | |||
| background: $govuk-brand-colour; | |||
| color: govuk-colour("white"); | |||
There was a problem hiding this comment.
Reminder - I don't think you need to specify the color (just the background) here.
|
|
||
| .inverse-background { | ||
| background: $govuk-brand-colour; | ||
| color: govuk-colour("white"); | ||
| } |
There was a problem hiding this comment.
I think since this has been moved into the helper you can remove it here.
There was a problem hiding this comment.
Ah yes, this should only be added one place.
sairamya93
force-pushed
the
move-speech-document
branch
from
cd61575 to
80036bf
Compare
| @@ -0,0 +1,31 @@ | |||
| class Speech < ContentItem | |||
| include NewsImage | |||
There was a problem hiding this comment.
I think this is added in a later commit? It should probably be included in the model in the commit it's added. The same goes for include Political
| include Political | ||
| include Updatable | ||
|
|
||
| def contributors |
There was a problem hiding this comment.
I think this method is missing a test
| content_store_hash.dig("details", "delivered_on") | ||
| end | ||
|
|
||
| def speech_type_explanation |
There was a problem hiding this comment.
This could probably do with a test too.
| @@ -0,0 +1,21 @@ | |||
| RSpec.describe SpeechPresenter do | |||
There was a problem hiding this comment.
These tests are great. 🎉
Could you add the original tests to the audit trail: https://github.com/alphagov/government-frontend/blob/122ae292fa540059ee165a94f8129d873c5205d5/test/presenters/speech_presenter_test.rb
| before do | ||
| { | ||
| "base_path" => "/a/base/path", | ||
| "details" => {}, | ||
| "links" => {}, | ||
| } | ||
| end |
There was a problem hiding this comment.
Is this needed? What does it do?
|
|
||
| <%= render "components/published_dates", { | ||
| published: display_date(@content_item.first_public_at), |
There was a problem hiding this comment.
I think this should call initial_publication_date rather than first_public_at. This was the use case I found for making initial_publication_date public.
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
spec/requests/speech_spec.rb
Outdated
| content_store_has_example_item("/government/speeches/vehicle-regulations", schema: :speech) | ||
| end | ||
|
|
||
| describe "GET index" do |
There was a problem hiding this comment.
This should be "GET show"
spec/requests/speech_spec.rb
Outdated
| end | ||
|
|
||
| describe "GET index" do | ||
| it "returns 200" do |
There was a problem hiding this comment.
As well as checking for a 200 response, it might be nice to add a test to check the template being rendered. I've found that this can catch general problems with the template, i.e. a call to an attribute that doesn't exist, and syntax errors.
e.g. https://github.com/alphagov/frontend/blob/main/spec/requests/landing_page_spec.rb#L37-L41
There was a problem hiding this comment.
Thanks for the reference test. I have added it
- Added Speech model that includes the related concerns for Speech[Model tests are included after adding the individual concerns] - 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
- 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 - Reference commit for political concern tests: 57bb3e1
- Modified any_updates? method with guard clause - Added a missing test in Updatable concern Add missing test for updatable concern
- Add tests for speech model and its related concerns
- To handle the translations in history_notice partial - Audit trail: https://github.com/alphagov/government-frontend/tree/90cf5386b165b3a342689744feece7c83449b153/config/locales
This is added as part of rendering important-metadata changes Audit trail for important-metadata change: alphagov/government-frontend@b1da197 For now, inverse-background is added as a helper and imported in application.scss file
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
80036bf to
62f0305
Compare
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