Description
Tomás Cohen Arazi (tcohen)
2019-07-01 14:55:27 UTC
Created attachment 91154 [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 Created attachment 91155 [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 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. :) (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 Created attachment 91361 [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> Created attachment 91362 [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> 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? (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 :) 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. It appears to work for me without the follow-up. (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! (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. 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'. 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? 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. 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... (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. (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. 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> 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> (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! Great. Will check later this week. 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> 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> Thank you, Marcel. Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.01 backported to 19.05.x for 19.05.06 |