Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

Prepare next_page column in pages table to be removed#791

Merged
lfdebrux merged 2 commits into
mainfrom
ldeb-remove-next-page-from-pages
Jul 9, 2025
Merged

Prepare next_page column in pages table to be removed#791
lfdebrux merged 2 commits into
mainfrom
ldeb-remove-next-page-from-pages

Conversation

@lfdebrux
Copy link
Copy Markdown
Contributor

@lfdebrux lfdebrux commented Jul 9, 2025

What problem does this pull request solve?

While working on migrating the forms models from forms-api to forms-admin I noticed that the next_page column for a page in the forms-api database is always set to nil.

As far as I can tell this column hasn't been used since before we migrated from grape (see PR #116), when we migrated to grape we switched to using the acts_as_list gem and the position column instead (see commit e8cc03).

I think we should remove this column now.

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?

theseanything
theseanything previously approved these changes Jul 9, 2025
lfdebrux added 2 commits July 9, 2025 14:02
This column hasn't been used since before we migrated from grape (see
PR #116), when we migrated to grape we switched to using the
acts_as_list gem and the position column instead (see commit e8cc03).

Let's remove this column now.
@lfdebrux lfdebrux force-pushed the ldeb-remove-next-page-from-pages branch from 3fc7b34 to 619da6a Compare July 9, 2025 11:03
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jul 9, 2025

lfdebrux added a commit to govuk-forms/forms-admin that referenced this pull request Jul 9, 2025
It turns out we don't need this column so we're removing it from the
database from forms-api (see PR govuk-forms/forms-api#791), let's remove it
from here as well.
@theseanything
Copy link
Copy Markdown
Contributor

@lfdebrux how come we're adding in tests that expect next_page to not be nil?

@lfdebrux
Copy link
Copy Markdown
Contributor Author

lfdebrux commented Jul 9, 2025

@lfdebrux how come we're adding in tests that expect next_page to not be nil?

So it's a bit confusing, but we do still rely on the next_page attribute in forms-runner and forms-admin, but it is generated by a method in https://github.com/alphagov/forms-api/blob/main/app/models/page.rb#L47-L49 that uses the position attribute, rather than coming from the database. So we don't need the next_page column, but we do need the next_page attribute in the JSON responses.

@lfdebrux lfdebrux merged commit 7134e3c into main Jul 9, 2025
4 checks passed
@lfdebrux lfdebrux deleted the ldeb-remove-next-page-from-pages branch July 9, 2025 11:42
lfdebrux added a commit to govuk-forms/forms-admin that referenced this pull request Jul 9, 2025
It turns out we don't need this column so we're removing it from the
database from forms-api (see PR govuk-forms/forms-api#791), let's remove it
from here as well.
lfdebrux added a commit to govuk-forms/forms-admin that referenced this pull request Jul 9, 2025
It turns out we don't need this column so we're removing it from the
database from forms-api (see PR govuk-forms/forms-api#791), let's remove it
from here as well.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants