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 after long lists #1631

Merged
merged 17 commits into from
Dec 6, 2024
Merged

Clean up after long lists #1631

merged 17 commits into from
Dec 6, 2024

Conversation

DavidBiddle
Copy link
Contributor

@DavidBiddle DavidBiddle commented Nov 26, 2024

What problem does this pull request solve?

Trello card: https://trello.com/c/BFWKD1ZG/2541-clean-up-after-long-lists-work

Includes:

  • removing the long_lists_enabled database column and the task for setting it.
  • removing the old selection type files and translations, since we don't need them any more
  • renaming references to long list selection (since the distinction doesn't make sense now that the old selection files have been removed). We can just say selection instead
  • renaming routes to remove references to long_list_selection
  • renaming the with_selections_settings trait to the singular with_selection_settings for consistency

Needs doing elsewhere:

  • Removing the logic that handles the old selection journey in forms-e2e-tests

Don't merge this until:

  • We've sent out the comms for the long lists feature
  • We're happy that we aren't going to revert the long lists feature

Note on how big this PR is

This PR is big in terms of the number of files touched and the number of commits. The changes themselves are fairly straightforward (largely deletions and renames), but there are a lot of them.

It would be easiest to go through it commit-by-commit.

If you reckon it's too unwieldy, let me know and I'll try to break it into a few PRs.

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Has all relevant documentation been updated?

@DavidBiddle DavidBiddle force-pushed the clean-up-after-long-lists branch 2 times, most recently from 4c77762 to 2fdd7f8 Compare November 27, 2024 11:22
@DavidBiddle DavidBiddle changed the title Clean up after long lists Do not merge: Clean up after long lists Nov 27, 2024
@DavidBiddle DavidBiddle force-pushed the clean-up-after-long-lists branch from 2fdd7f8 to ea86376 Compare November 27, 2024 14:27
@DavidBiddle DavidBiddle marked this pull request as ready for review December 2, 2024 15:11
alice-carr
alice-carr previously approved these changes Dec 2, 2024
Copy link
Contributor

@alice-carr alice-carr left a comment

Choose a reason for hiding this comment

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

I've been through each commit and this all makes sense, + the commit messages are clear 👍

@DavidBiddle DavidBiddle changed the title Do not merge: Clean up after long lists Clean up after long lists Dec 5, 2024
@DavidBiddle DavidBiddle force-pushed the clean-up-after-long-lists branch from ea86376 to 00c9b07 Compare December 5, 2024 16:30
Copy link
Contributor

@thomasiles thomasiles left a comment

Choose a reason for hiding this comment

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

thanks for taking the time to rename things and tidy up and also chop things into sensible commits 🏅

This flag is no longer used in the code, so we can remove it from the
groups database. We can also remove the rake tasks that turn the flag on
and off.
We've built new routes and controllers for configuring selection
questions, so the old ones are no longer used. This commit removes them.
Removes the selections_settings view, which is no longer used.

This also makes the 'selections_settings' translation block redundant,
so this commit also:
- removes most of the translations in this block altogether
- moves the translations which are still used elsewhere into more
appropriate blocks
We've removed the controller and the view for the old 'selections
settings' controller, so we can now safely get rid of the input object.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/type input object.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/options input object.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/bulk_options input object.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/type view.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/options view.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for the selection/bulk_options view.
We put the new selection files in 'long_lists_selection' folders, to
distinguish them from the old selection logic.

Since the old selection files have been removed, we no longer need this
distinction, so we can rename the `long_lists_selection` to just
`selection`.

This commit does the rename for all three selection controllers.
We used name scheme 'long_lists_selection_#{controller}_#{action}_path'
for the new selection routes, to distinguish them from the old selection
routes.

Since the old selection routes have been removed, we no longer need this
distinction, so we can use 'selection_#{controller}_#{action}_path' as
our name scheme instead.

This commit does the rename for the selection/type controller.
We used name scheme 'long_lists_selection_#{controller}_#{action}_path'
for the new selection routes, to distinguish them from the old selection
routes.

Since the old selection routes have been removed, we no longer need this
distinction, so we can use 'selection_#{controller}_#{action}_path' as
our name scheme instead.

This commit does the rename for the selection/options controller.
We used name scheme 'long_lists_selection_#{controller}_#{action}_path'
for the new selection routes, to distinguish them from the old selection
routes.

Since the old selection routes have been removed, we no longer need this
distinction, so we can use 'selection_#{controller}_#{action}_path' as
our name scheme instead.

This commit does the rename for the selection/bulk_options controller.
This test file uses variables to store the paths it's looking for. We
originally named these with `long_lists` to avoid a name collision with
the old selection routes. Those old routes no longer exist, so we can
rename the variables to make them simpler and more consistent with the
route names.
The old selection logic used the phrase 'selections settings' and
variations on that. That logic no longer exists, but the wording still
existed in the names of some tests (likely because they were copied or
adapted from old tests).

This commit changes this to 'selection options' which is the language
the newer logic uses.
The `with_selections_settings` trait was using the plural `selections`.
This was consistent with the old selection logic, but inconsistent with
both the other settings traits (e.g. `with_date_settings`) and the new
selection logic (which uses the singular `selection`).

This commit renames all instances of `with_selections_settings` to the
singular `with_selection_settings`.
@DavidBiddle DavidBiddle force-pushed the clean-up-after-long-lists branch from 00c9b07 to 4d347e1 Compare December 6, 2024 11:37
@DavidBiddle DavidBiddle enabled auto-merge December 6, 2024 11:37
@DavidBiddle DavidBiddle merged commit a86098f into main Dec 6, 2024
4 checks passed
@DavidBiddle DavidBiddle deleted the clean-up-after-long-lists branch December 6, 2024 11:41
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.

3 participants