Bug 33422

Summary:	ERM - Search header should change to match the section you are in
Product:	Koha	Reporter:	Pedro Amorim <pedro.amorim>
Component:	ERM	Assignee:	Matt Blenkinsop <matt.blenkinsop>
Status:	CLOSED FIXED	QA Contact:
Severity:	minor
Priority:	P5 - low	CC:	david, jacob.omara, jonathan.druart, jonathan.field, lucas, martin.renvoize, matt.blenkinsop, pedro.amorim
Version:	Main
Hardware:	All
OS:	All
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
Circulation function:
Bug Depends on:	32683, 32728
Bug Blocks:
Attachments:	Bug 33422: ERM search header should change Bug 33422: ERM search header should change Bug 33422: (bug 32728 follow-up) ERM - Update search header depending on navigation Bug 33422: Add cypress test

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 (tcohen) 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.