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

Prevent mutation after consumption #2028

Merged

Conversation

futa-ikeda
Copy link
Contributor

@futa-ikeda futa-ikeda commented Oct 11, 2023

  • Ticket: []
  • Feature flag: n/a

Purpose

Summary of Changes

  • Move logic to update query params before the search task
  • Rename onSearch argument to onQueryParamChange for more clarity

Screenshot(s)

Side Effects

QA Notes

@futa-ikeda futa-ikeda changed the title Prevent mutation after consumption; Change arg name to something more… Prevent mutation after consumption Oct 11, 2023
@coveralls
Copy link

coveralls commented Oct 11, 2023

Pull Request Test Coverage Report for Build 6615496946

  • 7 of 11 (63.64%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 70.651%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/osf-components/addon/components/search-page/component.ts 7 8 87.5%
lib/registries/addon/branded/discover/controller.ts 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
lib/osf-components/addon/components/search-page/component.ts 1 77.86%
Totals Coverage Status
Change from base Build 6615395637: -0.02%
Covered Lines: 5950
Relevant Lines: 8191

💛 - Coveralls

Copy link
Contributor

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

I think this basically makes sense. These types of problems are a pain.

@adlius
Copy link
Contributor

adlius commented Oct 13, 2023

Have you run the tests locally with the deprecation id removed from deprecation-workflow.js and deprecation breakpoints added to see if the tests hit the breakpoints?

@futa-ikeda
Copy link
Contributor Author

Have you run the tests locally with the deprecation id removed from deprecation-workflow.js and deprecation breakpoints added to see if the tests hit the breakpoints?

I had ran the test suite to only run tests pertaining to "search", but will rerun the entire test suite now, just to make sure. Verified that it was no longer logging the deprecations when using the search page as well.

@adlius
Copy link
Contributor

adlius commented Oct 20, 2023

Do we have a branch for post-release search work? If so, we'd probably want to merge this into that branch.

@futa-ikeda
Copy link
Contributor Author

I've just been merging things for the post-release stuff into develop. It should be going out early next week

@futa-ikeda futa-ikeda merged commit 1fd3177 into CenterForOpenScience:develop Oct 23, 2023
9 checks passed
@futa-ikeda futa-ikeda deleted the search-deprecation branch October 23, 2023 15:49
@futa-ikeda futa-ikeda added this to the 23.13.0 milestone Oct 23, 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.

4 participants