Description
Lucas Gass (lukeg)
2020-03-27 20:30:34 UTC
Why is is necessary to change the Bootstrap class to col-xs-12? $("#catalogue_detail_biblio").attr("class","col-xs-12"); We should probably just check for a normalized_isbn before building the AmazonCoverImages bit. Created attachment 102018 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Created attachment 102054 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Note from testing: Using Google Chrome or Chromium before the patch is applied: the local image is not displayed on the detail page (it is in Firefox - in the wrong position as per the test plan). After applying the patch it works the same in Firefox as it does in Google Chrome or Chromium. Created attachment 102056 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 102057 [details] [review] Bug 25007: (follow-up) Fix the way image check alters layout A question in the bug report asks: Why is is necessary to change the Bootstrap class to col-xs-12? The JavaScript which checks for the existence of an Amazon cover image is designed to remove the Amazon cover's container if there is none. Changing the class of cover image column is gone. However, the check is incorrect because it doesn't account for Local Covers. There might be both a 1 x 1 pixel Amazon image and a local cover image, so the layout shouldn't be changed unless there are no images present. To test, apply the patch and view the bibliographic details page of various titles including: - Amazon cover present, no local cover - No Amazon cover, local cover image present - Both Amazon and local cover image Created attachment 102060 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 102061 [details] [review] Bug 25007: (follow-up) Fix the way image check alters layout A question in the bug report asks: Why is is necessary to change the Bootstrap class to col-xs-12? The JavaScript which checks for the existence of an Amazon cover image is designed to remove the Amazon cover's container if there is none. Changing the class of cover image column is gone. However, the check is incorrect because it doesn't account for Local Covers. There might be both a 1 x 1 pixel Amazon image and a local cover image, so the layout shouldn't be changed unless there are no images present. To test, apply the patch and view the bibliographic details page of various titles including: - Amazon cover present, no local cover - No Amazon cover, local cover image present - Both Amazon and local cover image Signed-off-by: David Nind <david@davidnind.com> Created attachment 102062 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 102063 [details] [review] Bug 25007: (follow-up) Fix the way image check alters layout A question in the bug report asks: Why is is necessary to change the Bootstrap class to col-xs-12? The JavaScript which checks for the existence of an Amazon cover image is designed to remove the Amazon cover's container if there is none. Changing the class of cover image column is gone. However, the check is incorrect because it doesn't account for Local Covers. There might be both a 1 x 1 pixel Amazon image and a local cover image, so the layout shouldn't be changed unless there are no images present. To test, apply the patch and view the bibliographic details page of various titles including: - Amazon cover present, no local cover - No Amazon cover, local cover image present - Both Amazon and local cover image Signed-off-by: David Nind <david@davidnind.com> Should not it be done for XISBN as well? 701 [% FOREACH XISBN IN XISBNS %]<tr>[% IF ( AmazonCoverImages ) %]<td><a href="http://www.amazon.com/gp/reader/[% XISBN.normalized_isbn | uri %][% AmazonAssocTag | uri %]#reader-link"><img src="https://images- na.ssl-images-amazon.com/images/P/[% XISBN.normalized_isbn | html %].01._AA75_PU_PU-5_.jpg" /></a></td>[% END %] (In reply to Jonathan Druart from comment #12) > Should not it be done for XISBN as well? The FRBRizeEditions feature won't link a record if there is no ISBN, so I don't think it's possible for XISBN.normalized_isbn to evaluate as false AND a row be returned in the XISBN data. And what about OPAC? Should not we apply the same change to opac-detail? Created attachment 103034 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Nind <david@davidnind.com> Created attachment 103035 [details] [review] Bug 25007: (follow-up) Fix the way image check alters layout A question in the bug report asks: Why is is necessary to change the Bootstrap class to col-xs-12? The JavaScript which checks for the existence of an Amazon cover image is designed to remove the Amazon cover's container if there is none. Changing the class of cover image column is gone. However, the check is incorrect because it doesn't account for Local Covers. There might be both a 1 x 1 pixel Amazon image and a local cover image, so the layout shouldn't be changed unless there are no images present. To test, apply the patch and view the bibliographic details page of various titles including: - Amazon cover present, no local cover - No Amazon cover, local cover image present - Both Amazon and local cover image Signed-off-by: David Nind <david@davidnind.com> Created attachment 103036 [details] [review] Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt Comment on attachment 103036 [details] [review] Bug 25007: (follow-up) Add check for normalized ISBN in opac-detail.tt Obsoleting this. Do not write patch to make me happy ;) My feeling is that [OPAC]AmazonCoverImages syspref MUST be tested with normalized_isbn, always. Created attachment 103064 [details] [review] Bug 25007: Check for normaalized ISBN when building Amazon link TO RECREATE: -Have Amazon images turned on and local images too. -Have a record with a local cover image -Make sure the record has no ISBN in 020 field. -Load detail page of that record and see that the local cover images is not on the right hand side like it should be but forced underneath 'catalogue_detail_biblio' in an odd spot. -Apply patch and reload page -The image should be back in its normal spot Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 103065 [details] [review] Bug 25007: (follow-up) Fix the way image check alters layout A question in the bug report asks: Why is is necessary to change the Bootstrap class to col-xs-12? The JavaScript which checks for the existence of an Amazon cover image is designed to remove the Amazon cover's container if there is none. Changing the class of cover image column is gone. However, the check is incorrect because it doesn't account for Local Covers. There might be both a 1 x 1 pixel Amazon image and a local cover image, so the layout shouldn't be changed unless there are no images present. To test, apply the patch and view the bibliographic details page of various titles including: - Amazon cover present, no local cover - No Amazon cover, local cover image present - Both Amazon and local cover image Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 backported to 19.11.x for 19.11.06 backported to 19.05.x for 19.05.11 |