Bug 23246 - Record detail page jumps into the 'images' tab if no holdings
Summary: Record detail page jumps into the 'images' tab if no holdings
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 16507
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-01 14:55 UTC by Tomás Cohen Arazi
Modified: 2019-12-20 15:37 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.01


Attachments
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to (1.72 KB, patch)
2019-07-01 16:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23246: (follow-up) Hardcoding tab position is not enough (2.71 KB, patch)
2019-07-01 17:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to (1.77 KB, patch)
2019-07-05 18:55 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 23246: (follow-up) Hardcoding tab position is not enough (2.76 KB, patch)
2019-07-05 18:56 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to (1.74 KB, patch)
2019-11-25 17:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23246: (follow-up) Hardcoding tab position is not enough (2.81 KB, patch)
2019-11-25 17:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to (1.84 KB, patch)
2019-11-29 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23246: (follow-up) Hardcoding tab position is not enough (2.91 KB, patch)
2019-11-29 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-07-01 14:55:27 UTC
This is certainly related to bug 16507. The original implementation is supposed to jump into the 'media tab' if there are no holdings, but it is not checking HTML5MediaEnabled or HTML5MediaYouTube in order to jump to that tab.

Steps to reproduce:
- Have HTML5MediaYouTube and HTML5MediaEnabled disabled
- Create a record with no items
- Go to the detail page
=> FAIL: the 'Images' tab is selected
Comment 1 Tomás Cohen Arazi 2019-07-01 16:25:12 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-07-01 17:18:25 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2019-07-02 01:28:27 UTC
Comment on attachment 91155 [details] [review]
Bug 23246: (follow-up) Hardcoding tab position is not enough

Review of attachment 91155 [details] [review]:
-----------------------------------------------------------------

*sniff sniff* Love the fixing of the hard coding. :)
Comment 4 Tomás Cohen Arazi 2019-07-02 01:37:41 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 91155 [details] [review] [review]
> Bug 23246: (follow-up) Hardcoding tab position is not enough
> 
> Review of attachment 91155 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> *sniff sniff* Love the fixing of the hard coding. :)

Glad to hear
Comment 5 Maryse Simard 2019-07-05 18:55:59 UTC Comment hidden (obsolete)
Comment 6 Maryse Simard 2019-07-05 18:56:03 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2019-07-06 09:48:19 UTC
I am really not sure what this is trying to fix - please include a description of the problem and how to reproduce it in the test plan.

Should the media tab be preselected under certain conditions?
Comment 8 Katrin Fischer 2019-07-06 09:49:08 UTC
(In reply to Tomás Cohen Arazi from comment #0)
> This is certainly related to bug 16507. The original implementation is
> supposed to jump into the 'media tab' if there are no holdings, but it is
> not checking HTML5MediaEnabled or HTML5MediaYouTube in order to jump to that
> tab.
> 
> Steps to reproduce:
> - Have HTML5MediaYouTube and HTML5MediaEnabled disabled
> - Create a record with no items
> - Go to the detail page
> => FAIL: the 'Images' tab is selected

I think this was meant to say enabled? Because then this starts to make sense now :)
Comment 9 Katrin Fischer 2019-07-06 09:51:07 UTC
Only... it doesn't work.

I am looking at a detail page in staff and now the acq tab is preselected. Patches applied, restart_all and cache deleted.
Comment 10 Katrin Fischer 2019-07-06 09:52:06 UTC
It appears to work for me without the follow-up.
Comment 11 Tomás Cohen Arazi 2019-07-06 12:46:07 UTC
(In reply to Katrin Fischer from comment #9)
> Only... it doesn't work.
> 
> I am looking at a detail page in staff and now the acq tab is preselected.
> Patches applied, restart_all and cache deleted.

The original implementation jumped to the fourth tab in the event of no holdings.
The fourth tab is not the media one if HTML5MediaEnabled is disabled, and there are conditions in which even in that case it is not the fourth.

The follow-up just adds an ID to the media tab, and jumps into is by using the ID instead.

So, if it is not working, can you check on the page sources, and see if you have the ID, and the line that says:

$('#bibliodetails').tabs("option", "active", $('#media_tab').index() );

If it is there, then it is not working, otherwise your templates have been cached or similar.

I will retry myself. Thanks!
Comment 12 Tomás Cohen Arazi 2019-07-08 18:41:30 UTC
(In reply to Katrin Fischer from comment #10)
> It appears to work for me without the follow-up.
You need to have more than 3 tabs to highlight the bug.

1) Enable 'localcover', have 'HTML5MediaEnabled' and 'HTML5MediaYouTube' disabled
2) Search for 'a' on the staff interface
3) Pick a record with no items
=> FAIL: The detail page jumps to the 'Images' tab.
4) Apply this patches
5) Run (same command in kohadevbox and koha-testing-docker):
  $ restart_all
6) Reload the detail page
=> SUCCESS: The 'Holdings' tab is selected
7) On a separate staff interface tab, enable HTML5MediaEnabled and HTML5MediaYouTube (for the staff interface) <- I used YouTube so it is easier
8) Edit the MARC record, put a valid Youtube link on the 856$u field. Save.
=> SUCCESS: The detail page has the 'Media' tab picked.
Comment 13 Tomás Cohen Arazi 2019-07-08 18:47:19 UTC
This report tries to deal with:
- The template not respecting whether the HTML5MediaEnabled syspref is set to display the 'Media' tab, and makes the third tab to get selected

This was intended to work like this:
- You have HTML5MediaEnabled set to display the media tab
- The record has no items, then jump to the third tab

This is wrong because there are many situations that would yield to wrong tabs being selected, see #c2. Most of them are not trivial to reproduce, probably. But the most basic, having HTML5MediaEnabled disbaled, clearly fails.

So the patches attached:
- Check if we really have HTML5MediaEnabled set to display in the staff interface
- Assing an identifiable ID to the 'media' tab, so instead of a hardcoded tab index, it 'does the right thing'.
Comment 14 Marcel de Rooy 2019-07-19 06:26:47 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
   FAIL   forbidden patterns
                forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 899)

Please adjust your test plan too. Enabling the media pref is not enough; you should have stuff in 856 fields too, indicators, etc?
What I do see without your patch for a biblio without holdings, is a flash in staff from Acquisitions tab (==3) back to Holdings when loading detail.
Is solving 3 for media enough?
Comment 15 Katrin Fischer 2019-07-30 21:24:52 UTC
I agree with Marcel, but I also still see an issue for a record with URL, but without items: Tabs visible are holdings (0) and acquisitions. Acquisitions is selected (I think it should not be). One reason I think we should not jump is that we could still be displaying alternate holding information that is hidden this way.
Comment 16 Tomás Cohen Arazi 2019-11-25 17:14:09 UTC
Here again. This is the test plan I followed:

- Have a record with an 856 $uhttps://www.youtube.com/watch?v=uHav6PL_4Bo
- Have HTML5MediaYouTube and HTML5MediaEnabled disabled, and LocalCoverImages enabled
- Load the detail page of the record
=> FAIL: you see: https://snipboard.io/Csb7HK.jpg
(i.e. The syspref is disabled, but it jumps to the 'Images' tab, the fourth one, which is hardcoded and doesn't verify the syspref first).
- Apply this patches, restart_all and refresh the browser
=> SUCCESS: It doesn't jump to the 'Images' tab (https://snipboard.io/4z9NTl.jpg)
- Enable HTML5MediaYouTube and HTML5MediaEnabled
- Reload the detail page
=> SUCCESS:  The embedded media is displayed! (https://snipboard.io/REZ2T6.jpg)

The fact that it jumps to the 'Images' tab is a coincidence, if you have EasyAnalytics enabled and LocalCoverImages disabled, it will jump to the Analytics tab. This is because it is hardcoded to jump to the tab with index of 3. It is very easy to spot the problem in the diff...
Comment 17 Tomás Cohen Arazi 2019-11-25 17:16:07 UTC
(In reply to Marcel de Rooy from comment #14)
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
>    FAIL   forbidden patterns
>                 forbidden pattern: Do not use line breaks inside template
> tags (bug 18675) (line 899)
> 
> Please adjust your test plan too. Enabling the media pref is not enough; you
> should have stuff in 856 fields too, indicators, etc?

Done. I submitted a new plan, but beware that I'm fixing the negative case: i.e. the one in which context for displaying the media is not met, and yet there's a tab jump. So having 856 is not part of the requirements to test the wrong behavior I'm fixing. 

> What I do see without your patch for a biblio without holdings, is a flash
> in staff from Acquisitions tab (==3) back to Holdings when loading detail.
> Is solving 3 for media enough?

My follow-up adds an id and makes it jump to the ID, if present. Otherwise the current behavior is kept and I'm not dealing with it.
Comment 18 Tomás Cohen Arazi 2019-11-25 17:18:34 UTC
(In reply to Katrin Fischer from comment #15)
> I agree with Marcel, but I also still see an issue for a record with URL,
> but without items: Tabs visible are holdings (0) and acquisitions.
> Acquisitions is selected (I think it should not be). One reason I think we
> should not jump is that we could still be displaying alternate holding
> information that is hidden this way.

This is the problem my patches fix: don't jump if you don't need to. And if you do, jump to the right place. You really need to have more than 3 tabs to see it. That's why I mention EasyAnalytics.

I will explain again: the current code jumps to the 4th tab, no matter how the HTML5* sysprefs are set, or if the media tab is the fourth or another one.
Comment 19 Tomás Cohen Arazi 2019-11-25 17:42:49 UTC
Created attachment 95801 [details] [review]
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to

This patch makes jumping into the third tab happen only if
HTML5MediaEnabled is set to 'both' or 'staff'.

To test:
- Have HTML5MediaEnabled set to 'opac' or 'not at all'
- Open the detail page of a record with no holdings
=> FAIL: The 'images' tab is selected
- Apply this patch
- Reload
=> SUCCESS: The 'holdings' tab is selected as it should be.
- Sign off :-D

Sponsored-by: America Numismatic Society
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 20 Tomás Cohen Arazi 2019-11-25 17:42:54 UTC
Created attachment 95802 [details] [review]
Bug 23246: (follow-up) Hardcoding tab position is not enough

There are many conditions that determine the tab number for 'media'.
Translated into variables that hold the tabs count on each condition, it
is clear that '3' was not good enough:
            [%- holdings      = (SeparateHoldings)   ? 2 : 1
                notes         = (MARCNOTES || notes) ? 1 : 0
                acq           = (Koha.Preference('AcquisitionDetails')) ? 1 : 0
                frbr          = (FRBRizeEditions  && XISBNS) ? 1 : 0
                local_covers  = (LocalCoverImages && (localimages || CAN_user_tools_upload_local_cover_images)) ? 1 : 0
                media_tab     = holdings + notes + acq + frbr + local_covers
            -%]

This patch gets rid of the hardcoded value, adds an id to the 'media'
tab (i.e. 'media_tab') and uses JS to get the right index.

To test:
- Play with the different options governing those variables
=> FAIL: 3 is not always the 'media' tab
- Apply this patch
- Repeat your tests
=> SUCCESS: It always picks the 'media' tab
- Sign off :-D

Sponsored-by: American Numismatics Society
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 21 Tomás Cohen Arazi 2019-11-25 17:44:21 UTC
(In reply to Marcel de Rooy from comment #14)
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
>    FAIL   forbidden patterns
>                 forbidden pattern: Do not use line breaks inside template
> tags (bug 18675) (line 899)

Fixed this one!
Comment 22 Marcel de Rooy 2019-11-26 15:09:36 UTC
Great. Will check later this week.
Comment 23 Marcel de Rooy 2019-11-29 08:44:32 UTC
Created attachment 95872 [details] [review]
Bug 23246: Only jump into the 'images' tab HTML5MediaEnabled is set to

This patch makes jumping into the third tab happen only if
HTML5MediaEnabled is set to 'both' or 'staff'.

To test:
- Have HTML5MediaEnabled set to 'opac' or 'not at all'
- Open the detail page of a record with no holdings
=> FAIL: The 'images' tab is selected
- Apply this patch
- Reload
=> SUCCESS: The 'holdings' tab is selected as it should be.
- Sign off :-D

Sponsored-by: America Numismatic Society
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2019-11-29 08:44:36 UTC
Created attachment 95873 [details] [review]
Bug 23246: (follow-up) Hardcoding tab position is not enough

There are many conditions that determine the tab number for 'media'.
Translated into variables that hold the tabs count on each condition, it
is clear that '3' was not good enough:
            [%- holdings      = (SeparateHoldings)   ? 2 : 1
                notes         = (MARCNOTES || notes) ? 1 : 0
                acq           = (Koha.Preference('AcquisitionDetails')) ? 1 : 0
                frbr          = (FRBRizeEditions  && XISBNS) ? 1 : 0
                local_covers  = (LocalCoverImages && (localimages || CAN_user_tools_upload_local_cover_images)) ? 1 : 0
                media_tab     = holdings + notes + acq + frbr + local_covers
            -%]

This patch gets rid of the hardcoded value, adds an id to the 'media'
tab (i.e. 'media_tab') and uses JS to get the right index.

To test:
- Play with the different options governing those variables
=> FAIL: 3 is not always the 'media' tab
- Apply this patch
- Repeat your tests
=> SUCCESS: It always picks the 'media' tab
- Sign off :-D

Sponsored-by: American Numismatics Society
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Tomás Cohen Arazi 2019-11-29 08:47:48 UTC
Thank you, Marcel.
Comment 26 Martin Renvoize 2019-12-09 14:35:41 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Joy Nelson 2019-12-16 21:02:54 UTC
Pushed to 19.11.01
Comment 28 Lucas Gass 2019-12-20 15:37:40 UTC
backported to 19.05.x for 19.05.06