Migrate manuals from Manuals Frontend to Government Frontend #2424
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
1pretz1
force-pushed
the
migrate-manuals-code
branch
5 times, most recently
from
cd5fe3a to
d68af2b
Compare
jon-kirwan
requested changes
Apr 22, 2022
edwardkerry
reviewed
May 4, 2022
edwardkerry
reviewed
May 4, 2022
edwardkerry
reviewed
May 4, 2022
|
Excellent work @1pretz1, really appreciate the clear commit messages and history. I just have a few non-blocking admin questions and one suggestion for a possible test, but otherwise this large migration looks good. |
edwardkerry
reviewed
May 5, 2022
Adds the locales needed for manuals / sections / updates pages.
Adds manual keys to foreign locales. Command has also sorted some keys.
This is a module that will be shared across multiple different manual related presenters.
1pretz1
force-pushed
the
migrate-manuals-code
branch
from
d68af2b to
8b7fda8
Compare
|
Thanks for the reviews @jon-kirwan and @edwardkerry! I believe I've addressed all the comments. I couldn't see what the last comment by Ed referred to and so I assumed it was a suggestion of merging 7673030 into ebf79f5. Instead, I've moved the commits so they're next to each other, as I felt combining them cluttered the only locales we actually have values for (English). |
edwardkerry
approved these changes
May 12, 2022
edwardkerry
approved these changes
May 12, 2022
jon-kirwan
requested changes
May 13, 2022
Adds manual presenter and uses the shared Metadata and Manual module. We've also created a directory: views/content_items/manuals that will store the common partials for both manuals and HMRC manuals / sections / updates views.
The metadata assertion can now take multiple classes to search within, as extra options given to the component can add further classes, e.g by specifying `inverse: true`, `gem-c-metadata--inverse` is added to the wrapping div.
Also creates a couple of partials for `hmrc_sections` and `hmrc_header` as these templates will be re-used.
We'll be using the accordian in manual sections and updates pages.
**Note**: Test file for this module is being worked on seperately. This will included for both HMRC manual sections and normal manuals. Currently, we have to send extra requests to Content Store for data included in the parent manual for a section page. A better solution would be to include this data in the section at the point of publishing. We need to: * Add the same tagging to a section as a manual for contextual breadcrumbs (e.g `topics`). * Add the manual title to the HMRC section content item, as it only includes the base path.
We need to use and overwite manual.rb's page_title in a few places and instead of using `super` - I've opted for `alias_method` as it's easier to parse IMO. It also gives us the bonus of overwriting `page_title` and using the old `page_title` value elsewhere in a class.
Currently (in Manuals Frontend), the contextual breadcrumbs for section pages are generated via the parent manual content item. We have to do the same in Government Frontend as the tagging between the section and manual is different and so if we just used the section content item then we lose breadcrumbs.
The old `intro` method returned a load of new lines (\n) if headings and parapgraphs were removed, which was confusing as they ultimately get ignored when the HTML is rendered. Updates `intro` to return an empty string if only new lines are generated.
Update's pages don't exist as a content item in Content Store. The content item that is returned for the update's pages (/guidance/<manual>/updates or /hmrc-internal-manuals/<manual>/updates) is the parent manual content item and so we have to specify the templates and presenter directly, otherwise Government Frontend will try and use the parent content item's view and presenter. I've not added tests for these as the integration tests we're adding, and the ones that already present cover the new logic.
These will be used for both manual updates / hmrc manual updates pages.
1pretz1
force-pushed
the
migrate-manuals-code
branch
from
8b7fda8 to
b1f9f7f
Compare
|
Hey @jon-kirwan, thank you for the comments, really useful! I believe I've addressed them all now if you'd like to take another gander. |
jon-kirwan
approved these changes
May 13, 2022
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.
This PR migrates manuals, manual sections and updates pages to Government Frontend, from Manuals Frontend. HMRC manuals have also been migrated. I've treated each page as its own content item (with own views / presenters) as this follows convention within the application, even though the
/updatespages aren't actually represented as a content items in content store. I've also tried to reduce duplication by using presenter modules and view partials.I've left some to-do's which should be resolved at a later date. They mainly concern extra requests to Content Store for manual section pages, as they need data that is only available on the parent manual page. We should add the extra data needed to the manual section when it is being published, which would solve the issue.
Examples of these pages can be found below.
Manual
HMRC Manual
Testing this PR
You can test the PR yourself either locally or on integration. Local testing is simpler and you can do this by running the app your self against imported or live data (e.g
./startup --livescript then localhost:3090/guidance/some-manual-path) or using docker.For testing on integration you will need to deploy two branches and then publish a manual from Manuals Publisher. You can only test a normal manual, unless you publish a manual with your Signon organisation set to HMRC (instead of GDS).
Deploy to integration: Change rendering app to government-frontend manuals-publisher#1844
Deploy to integration: Migrate manuals from Manuals Frontend to Government Frontend #2424
Publish new manual: https://manuals-publisher.integration.publishing.service.gov.uk/
View on: https://www.integration.publishing.service.gov.uk/
Dependant on
Trello:
https://trello.com/c/YmmLFXu5/1127-manuals-migrate-manuals-frontend-code-to-government-frontend