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

[#7254] Remove PublicBody#notes #7269

Merged
merged 1 commit into from
Feb 2, 2023
Merged

[#7254] Remove PublicBody#notes #7269

merged 1 commit into from
Feb 2, 2023

Conversation

gbp
Copy link
Member

@gbp gbp commented Sep 2, 2022

Relevant issue(s)

Requires #6647
Fixes #7254

What does this do?

Remove PublicBody#notes

Why was this needed?

This has been replace by a separate Note model.

Notes to reviewer

Had wanted to use Factories over Fixtures for notes but too many specs needed updates and this wasn't really the right time to switch.

It seems the searching of public bodies by their note content on /body/list/all isn't working which is why a spec has been disabled. Will open an issue to restore this functionality.

@garethrees garethrees mentioned this pull request Sep 2, 2022
16 tasks
@gbp gbp added this to the Framework Modernisation milestone Jan 5, 2023
@gbp gbp removed the has-blockers label Jan 5, 2023
@gbp gbp self-assigned this Jan 5, 2023
@gbp gbp force-pushed the 7254-remove-legacy-notes branch 2 times, most recently from 359b4c6 to 3171a97 Compare January 6, 2023 13:59
@gbp gbp force-pushed the 7254-remove-legacy-notes branch 2 times, most recently from 765eca4 to 9e18843 Compare January 27, 2023 10:02
@gbp gbp marked this pull request as ready for review January 27, 2023 10:05
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

Couple of quick drive-by comments.

Not adverse to losing searching within notes, especially as we use them more. It's caused no end of UX issues, and I think we'd only really lose out where a body is known by a few different names (or has been renamed).

spec/controllers/general_controller_spec.rb Show resolved Hide resolved
app/models/public_body.rb Show resolved Hide resolved
@gbp
Copy link
Member Author

gbp commented Jan 27, 2023

Should drop temp:migrate_public_body_notes rake task too

This has been replace by a separate Note model.

Searching of public bodies by their note content on `/body/list/all` is
not possible due to the complex nature of the SQL query. A spec has been
disabled due it now failing without the old notes column.

This has been broken on WDTK for months without being noticed.

Fixes #7254
@gbp gbp force-pushed the 7254-remove-legacy-notes branch from dd24689 to 913ef20 Compare January 31, 2023 15:27
@gbp gbp merged commit 428459e into develop Feb 2, 2023
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.

Drop PublicBody#notes column and associated legacy code
2 participants