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

Remove business finder tech debt #1967

Merged
merged 8 commits into from Feb 13, 2020
Merged

Remove business finder tech debt #1967

merged 8 commits into from Feb 13, 2020

Conversation

@huwd
Copy link
Contributor

huwd commented Feb 6, 2020

Trello: https://trello.com/c/qBM5QLID/482-business-finder-tech-debt

Part of the work to clear up behind ourselves - this PR removes all references to the business finder.

Business finder is currently unpublished for users, but a bunch of config, tests and docs still remained.

@huwd huwd changed the title Remove business finder tech debt [WIP] Remove business finder tech debt Feb 6, 2020
@huwd huwd force-pushed the remove-business-finder-tech-debt branch 2 times, most recently from e208109 to 9de240c Feb 6, 2020
@huwd huwd force-pushed the remove-business-finder-tech-debt branch from 9de240c to e04f1c4 Feb 10, 2020
@huwd huwd force-pushed the remove-business-finder-tech-debt branch from e04f1c4 to 6a928d7 Feb 10, 2020
@huwd huwd changed the title [WIP] Remove business finder tech debt Remove business finder tech debt Feb 10, 2020
@huwd huwd requested review from vanitabarrett, koetsier and laurentqro Feb 10, 2020
@huwd huwd marked this pull request as ready for review Feb 10, 2020
Copy link
Contributor

vanitabarrett left a comment

There are quite a few instances of appear_in_find_eu_exit_guidance_business_finder still in the search-api codebase. I'm not sure what they were added for - we should confirm that it's ok to remove and delete if so.

Just one other comment about deleting the class associated with the tests we're removing.

require "spec_helper"
require "publishing_api_finder_publisher"

RSpec.describe PublishingApiFinderPublisher do

This comment has been minimized.

Copy link
@vanitabarrett

vanitabarrett Feb 10, 2020

Contributor

Looks like we're deleting tests for something which still exists here - should we be deleting the PublishingApiFinderPublisher class itself if it's no longer needed?

This comment has been minimized.

Copy link
@huwd

huwd Feb 12, 2020

Author Contributor

So i understand the class is needed. But was only tested through the medium of brexit business finder tests and advanced search tests...

... with both those features deprecated and removed, there are no tests left 😬...
We could try and work through what is left using it and write some new tests for that?

This comment has been minimized.

Copy link
@vanitabarrett

vanitabarrett Feb 12, 2020

Contributor

@huwd Do you know where this class is used/needed?

This comment has been minimized.

Copy link
@huwd

huwd Feb 12, 2020

Author Contributor

Humm i just checked again, I could have sworn i'd seen something else using the PublishingApiFinderPublisher class... but currently on the branch there's nothing.

I'll try removing it and re-running the tests.

This comment has been minimized.

Copy link
@huwd

huwd Feb 13, 2020

Author Contributor

Turns out tests pass... gonna put it on integration and click around a bunch... check nothing is weird... I'm still super suspicious

@laurentqro

This comment has been minimized.

Copy link
Contributor

laurentqro commented Feb 11, 2020

There are quite a few instances of appear_in_find_eu_exit_guidance_business_finder still in the search-api codebase. I'm not sure what they were added for - we should confirm that it's ok to remove and delete if so.

It looks like the appear_in_find_eu_exit_guidance_business_finder checkbox approach was replaced the facet_groups approach:

The finder content schema needs to have appear_in_find_eu_exit_guidance_business_finder removed too: https://github.com/alphagov/govuk-content-schemas/blob/master/formats/shared/definitions/finder.jsonnet#L51.

@laurentqro

This comment has been minimized.

Copy link
Contributor

laurentqro commented Feb 11, 2020

For some additional context about facet_groups: alphagov/govuk-content-schemas#878.

@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Feb 12, 2020

There are quite a few instances of appear_in_find_eu_exit_guidance_business_finder still in the search-api codebase. I'm not sure what they were added for - we should confirm that it's ok to remove and delete if so.

Just one other comment about deleting the class associated with the tests we're removing.

@koetsier, humm - you know a bit more about this than I do i think. Can i double check removing this stuff (and all it's references in schema configs isn't related to the transition checkbox?

... I can't see any overlap, but would like a second opinion?

@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Feb 12, 2020

There are quite a few instances of appear_in_find_eu_exit_guidance_business_finder still in the search-api codebase. I'm not sure what they were added for - we should confirm that it's ok to remove and delete if so.

It looks like the appear_in_find_eu_exit_guidance_business_finder checkbox approach was replaced the facet_groups approach:

* [alphagov/finder-frontend@6f484a6](https://github.com/alphagov/finder-frontend/commit/6f484a634)

* https://github.com/alphagov/search-api/pull/1535/files

The finder content schema needs to have appear_in_find_eu_exit_guidance_business_finder removed too: https://github.com/alphagov/govuk-content-schemas/blob/master/formats/shared/definitions/finder.jsonnet#L51.

Addressed these:

@huwd huwd requested a review from vanitabarrett Feb 13, 2020
@huwd huwd merged commit 1555d06 into master Feb 13, 2020
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
@huwd huwd deleted the remove-business-finder-tech-debt branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.