Bug 33422 - ERM - Search header should change to match the section you are in
Summary: ERM - Search header should change to match the section you are in
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Matt Blenkinsop
QA Contact:
URL:
Keywords:
Depends on: 32683 32728
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-05 14:13 UTC by Pedro Amorim
Modified: 2023-12-28 20:43 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.05


Attachments
Bug 33422: ERM search header should change (3.93 KB, patch)
2023-04-05 15:34 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 33422: ERM search header should change (3.96 KB, patch)
2023-04-05 16:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 33422: (bug 32728 follow-up) ERM - Update search header depending on navigation (4.12 KB, patch)
2023-04-06 06:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33422: Add cypress test (2.03 KB, patch)
2023-04-06 09:38 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-04-05 14:13:24 UTC
Originally fixed in bug 32728 but it's not working again, we need to fix it, and provide tests for it to make sure this doesn't happen again.
Comment 1 Matt Blenkinsop 2023-04-05 15:34:35 UTC
Created attachment 149220 [details] [review]
Bug 33422: ERM search header should change

This patch removes reference to a non-existent id ("ui-id-X") and replaces it with new ids specific to agreements, titles and packages.

Test plan:
1) Apply patch
2) Navigate between Agreements, Packages, Titles using the sidebar. The search bar should change each time to match the section
3) Add some agreemens, packages, titles
4) Perform a search using the different search bar options. Each time it should take you to the correct page with the search activated in the KohaTable. The navbar should also be highlighted to match the correct section and the top search bar should also show the correct section
Comment 2 David Nind 2023-04-05 16:49:44 UTC
Created attachment 149221 [details] [review]
Bug 33422: ERM search header should change

This patch removes reference to a non-existent id ("ui-id-X") and replaces it with new ids specific to agreements, titles and packages.

Test plan:
1) Apply patch
2) Navigate between Agreements, Packages, Titles using the sidebar. The search bar should change each time to match the section
3) Add some agreemens, packages, titles
4) Perform a search using the different search bar options. Each time it should take you to the correct page with the search activated in the KohaTable. The navbar should also be highlighted to match the correct section and the top search bar should also show the correct section

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2023-04-05 16:50:57 UTC
Testing notes (using KTD):

1. After applying the patch (step 1), run: yarn build
Comment 4 Jonathan Druart 2023-04-06 06:52:07 UTC
Bug 32683 certainly caused this regression.
Comment 5 Jonathan Druart 2023-04-06 06:54:56 UTC
Created attachment 149224 [details] [review]
Bug 33422: (bug 32728 follow-up) ERM - Update search header depending on navigation

This patch removes reference to a non-existent id ("ui-id-X") and replaces it with new ids specific to agreements, titles and packages.

Test plan:
1) Apply patch
2) Navigate between Agreements, Packages, Titles using the sidebar. The search bar should change each time to match the section
3) Add some agreemens, packages, titles
4) Perform a search using the different search bar options. Each time it should take you to the correct page with the search activated in the KohaTable. The navbar should also be highlighted to match the correct section and the top search bar should also show the correct section

Signed-off-by: David Nind <david@davidnind.com>

JD Amended commit message: link with 32728
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2023-04-06 06:55:23 UTC
Not blocker - we should add tests for this.
Comment 7 Matt Blenkinsop 2023-04-06 08:32:00 UTC
I need to get used to Cypress so I can have a look at writing some tests
Comment 8 Matt Blenkinsop 2023-04-06 09:38:39 UTC
Created attachment 149227 [details] [review]
Bug 33422: Add cypress test

This patch adds a cypress test to check whether the "active" class is correctly updating when navigating
Comment 9 Pedro Amorim 2023-04-06 11:30:43 UTC
These look good, some points for discussion:
1) Should we make this bug depend on bug 33408 and have these tests be written in that fashion? i.e. mock ERMModule sys pref.
2) Should we move Dialog_spec.ts and this new Searchbar_spec.ts into a single ERM/Common_spec.ts just for organization sake.
Comment 10 Jonathan Druart 2023-04-06 12:01:38 UTC
(In reply to Pedro Amorim from comment #9)
> These look good, some points for discussion:
> 1) Should we make this bug depend on bug 33408 and have these tests be
> written in that fashion? i.e. mock ERMModule sys pref.
> 2) Should we move Dialog_spec.ts and this new Searchbar_spec.ts into a
> single ERM/Common_spec.ts just for organization sake.

This (33422) should go first, as it's a bugfix. We will need to adjust the other bug on top of it.

I would suggest ERM/Regressions_spec.ts, we have one "regressions" for selenium and it's good to group those small tests together IMO.
Comment 11 Tomás Cohen Arazi 2023-04-06 12:30:47 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Jacob O'Mara 2023-04-16 20:38:49 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.