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

Clean up legacy code for document collections #8442

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

mtaylorgds
Copy link
Contributor

What

Remove vestigial code that has been left behind after the migration of the document collection pages to the GOV.UK Design System.

Trello

Trello card.

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

Follow these steps if you are doing a Rails upgrade.

Copy link
Contributor

@farahTW farahTW Nov 2, 2023

Choose a reason for hiding this comment

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

Just one query on this feature test, are we supposed to remove these feature tests?
or just remove annotations @javascript @design-system-wip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deleted tests are for the old "half-way to design system" pages where it was all done with Javascript on a single page. The functionality is covered by the other tests in the file that were added when we updated the flow with extra pages.

@davidtrussler
Copy link
Contributor

davidtrussler commented Nov 2, 2023

I'm not sure if we are planning to remove all files under javascripts/admin_legacy and stylesheets/admin_legacy in one go, though to date the process has been to remove JavaScript and CSS files as and when they become surplus to requirements, which is probably the better way to go IMO.

If that is the case then we should also delete these files at this point:

  • app/assets/javascripts/admin_legacy/_document_group_ordering.js
  • app/assets/javascripts/admin_legacy/document_finder.js
  • app/assets/javascripts/admin_legacy/document_collection.js
  • app/assets/stylesheets/admin_legacy/views/_document-collection-groups.scss
  • app/assets/stylesheets/admin_legacy/helpers/_document-finder.scss

@davidtrussler
Copy link
Contributor

davidtrussler commented Nov 2, 2023

Just a query on the status of this, and one other, PR: is this one based on PR#8415 ("Prepare release for document collections") and can that one therefore be closed? This seems to be an extended duplicate of that work.

@mtaylorgds
Copy link
Contributor Author

Just a query on the status of this, and one other, PR: is this one based on PR#8415 ("Prepare release for document collections") and can that one therefore be closed? This seems to be an extended duplicate of that work.

By necessity, this PR had to be branched off of PR#8415 (since we can't cleanup until that work has been merged). The intention is that once that PR is merged this one can be rebased off of the latest main, and so will only contain the "remove" commits.

@mtaylorgds
Copy link
Contributor Author

I'm not sure if we are planning to remove all files under javascripts/admin_legacy and stylesheets/admin_legacy in one go, though to date the process has been to remove JavaScript and CSS files as and when they become surplus to requirements, which is probably the better way to go IMO.

If that is the case then we should also delete these files at this point:

  • app/assets/javascripts/admin_legacy/_document_group_ordering.js
  • app/assets/javascripts/admin_legacy/document_finder.js
  • app/assets/javascripts/admin_legacy/document_collection.js
  • app/assets/stylesheets/admin_legacy/views/_document-collection-groups.scss
  • app/assets/stylesheets/admin_legacy/helpers/_document-finder.scss

It would be nice to remove unnecessary files as and when we go (ultimately leading to the removal of everything under javascripts/admin_legacy and stylesheets/admin_legacy).

In this case those files are also used by some files under statistics_announcements, which I believe are ultimately not used anymore, but given the complexity of so many interdependencies I thought it best to do as a separate ticket—see the "cleanup document_searches code" Trello card.

@davidtrussler
Copy link
Contributor

I'm not sure if we are planning to remove all files under javascripts/admin_legacy and stylesheets/admin_legacy in one go, though to date the process has been to remove JavaScript and CSS files as and when they become surplus to requirements, which is probably the better way to go IMO.
If that is the case then we should also delete these files at this point:

  • app/assets/javascripts/admin_legacy/_document_group_ordering.js
  • app/assets/javascripts/admin_legacy/document_finder.js
  • app/assets/javascripts/admin_legacy/document_collection.js
  • app/assets/stylesheets/admin_legacy/views/_document-collection-groups.scss
  • app/assets/stylesheets/admin_legacy/helpers/_document-finder.scss

It would be nice to remove unnecessary files as and when we go (ultimately leading to the removal of everything under javascripts/admin_legacy and stylesheets/admin_legacy).

In this case those files are also used by some files under statistics_announcements, which I believe are ultimately not used anymore, but given the complexity of so many interdependencies I thought it best to do as a separate ticket—see the "cleanup document_searches code" Trello card.

Yeah that's fine, as long as it's accounted for somewhere. I traced some of that back to Statistics Announcements as well: it looked like there are some partials in there - that call the JS above - that should be removed but it looks like you have caught those in that ticket. 👍

@farahTW
Copy link
Contributor

farahTW commented Nov 7, 2023

LGTM

Remove the POST endpoint for adding pages that are not published by
 Whitehall to a document collection group.

Removes the:
- route
- controller action.

There were no controller tests for this action.
Remove the POST endpoint for updating the memberships of a document
 collection group.

Removes the:
- route
- controller action
- controller actions tests.
Rails will render the appropriate view by default, so there's no need to
 call render from within the controller action method to render the
 default view.

In `confirm_destroy`, the return is unnecessary now that there is no
 more code after the potential redirect.
@minhngocd minhngocd force-pushed the 584_clean-up-legacy-code-for-document-collections branch from 01214e3 to d1c66be Compare November 8, 2023 11:34
- Removed redundant views that were only used in the legacy views
- Removed redundant destroy endpoint in routes and reorganised the routes config
Along with some minor refactorings
@minhngocd minhngocd force-pushed the 584_clean-up-legacy-code-for-document-collections branch from 163e407 to 2004cdf Compare November 8, 2023 13:50
@minhngocd minhngocd marked this pull request as ready for review November 8, 2023 13:50
@minhngocd
Copy link
Contributor

Added further removals to the views, removed an unused route, and fixed the route referrals in the code.

Copy link
Contributor

@RodneyJohnsonGDS RodneyJohnsonGDS left a comment

Choose a reason for hiding this comment

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

LGTM!

@minhngocd minhngocd merged commit d12c7a9 into main Nov 9, 2023
15 checks passed
@minhngocd minhngocd deleted the 584_clean-up-legacy-code-for-document-collections branch November 9, 2023 11:50
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.

5 participants