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.

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

Jump to bottom

Methodology page #57

Merged
merged 30 commits into from
Jan 9, 2025
Merged

Methodology page #57

merged 30 commits into from
Jan 9, 2025

Conversation

kacperpONS
Copy link
Contributor

@kacperpONS kacperpONS commented Dec 10, 2024 •
edited
Loading

What is the context of this PR?

Jira card: https://jira.ons.gov.uk/browse/CMS-200
Functional Walkthrough: https://www.loom.com/share/2c0397aaa73e43ceaaa2bff6baa4bd91?sid=ac7ca831-9d45-4b2c-aac7-1e66e52a32bf

Added the Methodology page model.
Refactored the Section block for it to be reused between the Analysis and Methodology pages.
Added unit tests.

How to review

Run the app locally try to add a methodology page under a topic page and make sure that all the expected panels in the admin view and are there and are displayed on the page

zerolab
zerolab reviewed Dec 10, 2024
View reviewed changes
cms/core/blocks/stream_blocks.py Show resolved Hide resolved
cms/core/blocks/section_block.py Outdated Show resolved Hide resolved
@kacperpONS kacperpONS changed the title Refactorig to make the Section block reusable between apps Methodology page Dec 11, 2024
@kacperpONS kacperpONS marked this pull request as ready for review December 11, 2024 14:18
@kacperpONS kacperpONS requested a review from a team as a code owner December 11, 2024 14:18
zerolab
zerolab requested changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Solid start. Left a number of notes, questions and suggestions.
Will re-review (and do functional testing) around 4pm

cms/core/blocks/section_blocks.py Outdated Show resolved Hide resolved
cms/core/blocks/section_blocks.py Outdated Show resolved Hide resolved
cms/core/blocks/stream_blocks.py Show resolved Hide resolved
cms/jinja2/templates/pages/methodology_page.html Outdated Show resolved Hide resolved
cms/jinja2/templates/pages/methodology_page.html Outdated Show resolved Hide resolved
cms/standard_pages/models.py Outdated Show resolved Hide resolved
cms/standard_pages/models.py Outdated Show resolved Hide resolved
cms/standard_pages/tests/factories.py Outdated Show resolved Hide resolved
cms/standard_pages/tests/test_models.py Outdated Show resolved Hide resolved
cms/standard_pages/models.py Outdated Show resolved Hide resolved
zerolab
zerolab reviewed Dec 11, 2024
View reviewed changes
cms/methodology/models.py Outdated Show resolved Hide resolved
zerolab
zerolab reviewed Dec 11, 2024
View reviewed changes
cms/methodology/models.py Outdated Show resolved Hide resolved
zerolab
zerolab reviewed Jan 6, 2025
View reviewed changes
cms/jinja2/templates/pages/methodology_page.html Outdated Show resolved Hide resolved
cms/methodology/tests/factories.py Outdated Show resolved Hide resolved
zerolab
zerolab requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Did another review. A couple of minor changes and we should be good to go

zerolab and others added 5 commits January 8, 2025 10:43
@zerolab
Merge branch 'main' into methodology-page-CMS-200
ff99288
@zerolab
Tidy up tests
2141b87
@zerolab
Make related publications specific to Methodology page and limited to…
028d806 
… Analysis pages
@zerolab
Minor tidy ups
a498b4d 
Keeps the default_auto_field to AutoField and cleans up
the use of RelatedLinksBlock in SectionContentBlock as
it is already initialised with RelatedContentBlock
@zerolab
Merge branch 'main' into methodology-page-CMS-200
5b58b6e
@zerolab zerolab requested a review from RealOrangeOne January 8, 2025 15:46
@zerolab zerolab mentioned this pull request Jan 8, 2025
Merged
@nehakerung nehakerung self-requested a review January 9, 2025 10:27
nehakerung
nehakerung reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@nehakerung nehakerung left a comment

Choose a reason for hiding this comment

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

👀 Just had a look and when saving as a draft or publish i get Screenshot 2025-01-09 at 10 15 57
The page saves but then you can't access the page again, getting the same error.

lmk if it's just me 🫡

@zerolab
Copy link
Contributor

zerolab commented Jan 9, 2025

if you tested the PR locally before:

  1. on your local machine run git checkout ff99288
  2. remove any existing methodology pages via the Wagtail admin
  3. in the container: django-admin migrate methodology zero
  4. on your local machine: git checkout methodology-page-CMS-200

@zerolab
Copy link
Contributor

zerolab commented Jan 9, 2025 •
edited
Loading

@kacperpONS could you please revert 186f966 as it breaks the linter? sorted

@nehakerung nehakerung self-requested a review January 9, 2025 11:09
nehakerung
nehakerung approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@nehakerung nehakerung left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - all works and looks good; just the checks need passing now🥲 Maybe a loom recording too

@zerolab zerolab force-pushed the methodology-page-CMS-200 branch from 8ab6ac1 to 765705e Compare January 9, 2025 11:37
@kacperpONS
Copy link
Contributor Author

I've reviewed Dan's changes and everything is looking good 👍

zerolab
zerolab approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Approving as I've previously requested changes and GitHub is picky

@zerolab zerolab requested a review from nehakerung January 9, 2025 16:07
@zerolab zerolab merged commit c9c094a into main Jan 9, 2025
8 checks passed
@zerolab zerolab deleted the methodology-page-CMS-200 branch January 9, 2025 16:23
sanjeevz3009
sanjeevz3009 reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

Reviewed!

ababic added a commit that referenced this pull request Jan 13, 2025
@ababic
Merge branch 'main' into feature/private-media
5073ec5 
* main:
  Remove the analysis app (#71)
  Fix to related content on the release page (#72)
  Fix for  equations and embeds rendering on Methodology page(#74)
  Update readme with extra steps needed for pre-commit (#67)
  Remove the analysis models (#70)
  Add an articles app for statistical articles (#69)
  Methodology page (#57)
  Ensure the default DB is **always** used for writes (#68)
  Styling for the release page plus some refactoring (#55)
  Bundles: exclude Release Calendar pages with a date in the past (#66)
  Disable markup from megalinter (#64)
  Ignore vscode workspace settings files (#65)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanjeevz3009 sanjeevz3009 sanjeevz3009 left review comments

@zerolab zerolab zerolab approved these changes

@nehakerung nehakerung nehakerung approved these changes

@RealOrangeOne RealOrangeOne Awaiting requested review from RealOrangeOne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kacperpONS @nehakerung @zerolab @RealOrangeOne @sanjeevz3009