Bug 32302 - "ISBN" label shows when no ISBN data present when sending list
Summary: "ISBN" label shows when no ISBN data present when sending list
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-21 05:58 UTC by David Cook
Modified: 2023-12-28 20:44 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes email messages sent when sending lists so that if there are no ISBNs for a record, an empty label is not shown.
Version(s) released in:
23.05.00,22.11.01,22.05.09, 21.11.16


Attachments
Bug 32302: Hide "ISBN" label when no ISBN data when sending list (1.56 KB, patch)
2022-11-30 22:56 UTC, David Cook
Details | Diff | Splinter Review
Bug 32302: Hide "ISBN" label when no ISBN data when sending list (1.61 KB, patch)
2022-11-30 23:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 32302: (follow-up) Hide ISBN label when list sent from the staff interface (1.23 KB, patch)
2022-11-30 23:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 32302: Hide "ISBN" label when no ISBN data when sending list (1.71 KB, patch)
2022-12-09 10:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32302: (follow-up) Hide ISBN label when list sent from the staff interface (1.33 KB, patch)
2022-12-09 10:51 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 David Cook 2022-11-21 05:58:59 UTC
There's a template toolkit bug in the condition for BIBLIO_RESULT.ISBN in ./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt so "ISBN:" shows in the email whether or not there is any ISBN data.
Comment 1 Adam Styles 2022-11-30 10:53:38 UTC
Hi David,

Just to clarify the issues of bug raised:


By "ISBN:" shows in the email whether or not there is any ISBN data." 

Are you suggesting that, IF a bibliographic record, has NO ISBN FIELD in the marc file? that the output resulted in email form, still displays a blank ISBN label.

OR that...

IF a bibliographic record, has an empty ISBN FIELD value in the marc file? that the output resulted in email form, still displays a blank ISBN label.

The reason I ask, is that in the file:
./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt

There is a specific IF statement line 61 - 68

```
[% IF ( BIBLIO_RESULT.ISBN ) %]
<span>
                ISBN: [% FOREACH isbn IN BIBLIO_RESULT.ISBN %]
                          [% isbn | $raw %]
                          [% UNLESS ( loop.last ) %]; [% END %]
                      [% END %]
            </span><br/>
[% END %]

```
Comment 2 Adam Styles 2022-11-30 11:06:20 UTC
Hi David,

Just to clarify the bug raised:

```
"ISBN:" shows in the email whether or not there is any ISBN data." 
```

Are you suggesting that, IF a bibliographic record, has NO ISBN FIELD in the marc file? that the output result in email, still displays a blank ISBN label.

OR that...

IF a bibliographic record, has an empty ISBN FIELD with no value in subfields, in the marc file, that the output result in email, still displays a blank ISBN label.

The reason I ask, is that in the file:
./koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt

There is a specific IF statement line 61 - 68, that will only display the ISBN output IF the marc file itself has an ISBN marc field present (either subfield populated or not).

See below

```
[% IF ( BIBLIO_RESULT.ISBN ) %]
<span>
    ISBN: [% FOREACH isbn IN BIBLIO_RESULT.ISBN %]
              [% isbn | $raw %]
              [% UNLESS ( loop.last ) %]; [% END %]
          [% END %]
</span><br/>
[% END %]
```

I ran a quick test in opac which appears to show the ISBN label, only when the marc record itself has a 020 ISBN field. When I removed 020 ISBN field in test marc record, the ISBN label did not populate in output results.

Are you able to create a test plan based on your perspective on this bug? IF so I can test authentically.

Thanks.
Comment 3 David Cook 2022-11-30 22:51:12 UTC
Hi Adam,

Thanks for taking an interest in this one. I'm not sure that I 100% follow your comment. 

The premise is there is a List with bib records that don't have the 020 MARC field in them. If you view the list in the OPAC, click "Send list", and email the list to yourself, the email should have blank "ISBN:" lines in the email for the records that don't have 020 MARC fields in them. 

This is because BIBLIO_RESULT.ISBN contains an empty array reference which counts as a "true" value in [% IF ( BIBLIO_RESULT.ISBN ) %].

That IF statement needs to check the size/length of the array reference in the ISBN field of the BIBLIO_RESULT hash. 

I've already fixed this one locally, so it hasn't been a high priority for me, but I'll attach a patch in a minute.
Comment 4 David Cook 2022-11-30 22:56:49 UTC
Created attachment 144366 [details] [review]
Bug 32302: Hide "ISBN" label when no ISBN data when sending list

There is a logic mistake in the opac-sendshelf.tt template where
we test if the "ISBN" key exists in the "BIBLIO_RESULT" hash when
we should really be testing if the arrayref returned by "ISBN" is
empty or not, since that "ISBN" key always exists and contains at least
an empty arrayref.

Test plan:
1. Apply patch and restart your Koha Starman
2. Create a public list containing records with 020 MARC fields and
records without 020 MARC fields
3. View the list in the OPAC
4. Click "Send list" and email it to yourself
5. Note that only records with 020 MARC fields with show "ISBN:"
in the email output
Comment 5 David Nind 2022-11-30 23:55:52 UTC
Created attachment 144368 [details] [review]
Bug 32302: Hide "ISBN" label when no ISBN data when sending list

There is a logic mistake in the opac-sendshelf.tt template where
we test if the "ISBN" key exists in the "BIBLIO_RESULT" hash when
we should really be testing if the arrayref returned by "ISBN" is
empty or not, since that "ISBN" key always exists and contains at least
an empty arrayref.

Test plan:
1. Apply patch and restart your Koha Starman
2. Create a public list containing records with 020 MARC fields and
records without 020 MARC fields
3. View the list in the OPAC
4. Click "Send list" and email it to yourself
5. Note that only records with 020 MARC fields with show "ISBN:"
in the email output

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2022-11-30 23:55:58 UTC
Created attachment 144369 [details] [review]
Bug 32302: (follow-up) Hide ISBN label when list sent from the staff interface

This makes the same change to the staff interface. Lists sent
from the staff interface hide the ISBN label in the email message
when there are no ISBNs for a record.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-12-01 00:06:29 UTC
Hi David.

I added a follow-up for lists sent from the staff interface, as they were doing exactly the same thing - I didn't read the test plan correctly, and wondered why it wasn't working from the staff interface.

Hope you don't mind!

David

Testing notes using KTD (koha-testing-docker):

1. Created a list from the search results for 'perl' (11 results, 2 don't have ISBNs, some results have multiple ISBNs).

2. To setup KTD to send email using a Google account:
   2.1 Edit your /etc/koha/sites/kohadev/koha-conf.xml file
   2.2 Add this configuration near the end:
      . <user_name> = your Google email address
      . <password> = an APP password that you need to create in your Google account, not your Google account password

  <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>GOOGLEACCOUNTUSER</user_name>
    <password>GOOGLEAPPPASSWORD</password>
    <debug>1</debug>
  </smtp_server>
Comment 8 David Cook 2022-12-01 00:37:58 UTC
(In reply to David Nind from comment #7)
> Hope you don't mind!

Not at all! Thanks for testing and for the follow-up :).
Comment 9 Adam Styles 2022-12-01 00:46:07 UTC
Hi David and David,

Firstly, apologies if my questions on your initial bug report were not as coherent as they were in my head lol. I am new to the Koha dev community and will work on my communication skills.

Thanks for the patch,  I was looking for something to fix and this looked like a quick one, I will look around for others 8-)

Thanks again.
Comment 10 David Cook 2022-12-01 01:06:27 UTC
(In reply to Adam Styles from comment #9)
> Firstly, apologies if my questions on your initial bug report were not as
> coherent as they were in my head lol. I am new to the Koha dev community and
> will work on my communication skills.

No worries at all. I think my initial description was pretty vague! 

Glad to have you aboard. Always good to see other Australians working on Koha! It's not often I get to share a timezone with another Koha dev ;).
 
> Thanks for the patch,  I was looking for something to fix and this looked
> like a quick one, I will look around for others 8-)

Good instinct. It was a quick one! 

If you're looking for easy bugs, you might considering doing a Bugzilla search on the keyword "Academy".
Comment 11 Marcel de Rooy 2022-12-09 10:51:07 UTC
Created attachment 144526 [details] [review]
Bug 32302: Hide "ISBN" label when no ISBN data when sending list

There is a logic mistake in the opac-sendshelf.tt template where
we test if the "ISBN" key exists in the "BIBLIO_RESULT" hash when
we should really be testing if the arrayref returned by "ISBN" is
empty or not, since that "ISBN" key always exists and contains at least
an empty arrayref.

Test plan:
1. Apply patch and restart your Koha Starman
2. Create a public list containing records with 020 MARC fields and
records without 020 MARC fields
3. View the list in the OPAC
4. Click "Send list" and email it to yourself
5. Note that only records with 020 MARC fields with show "ISBN:"
in the email output

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2022-12-09 10:51:11 UTC
Created attachment 144527 [details] [review]
Bug 32302: (follow-up) Hide ISBN label when list sent from the staff interface

This makes the same change to the staff interface. Lists sent
from the staff interface hide the ISBN label in the email message
when there are no ISBNs for a record.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Tomás Cohen Arazi 2022-12-13 12:27:28 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 14 Martin Renvoize 2022-12-15 13:37:32 UTC
Nice work everyone!

Pushed to 22.11.x for the next release
Comment 15 Lucas Gass 2023-01-19 22:55:03 UTC
Backported to 22.05.x for upcoming 22.05.09
Comment 16 Arthur Suzuki 2023-01-26 10:25:34 UTC
applied to 21.11.x for 21.11.16
Comment 17 wainuiwitikapark 2023-03-15 01:30:42 UTC
Not backported to 21.05.x