Bug 11492 - Receiving a serial item causes routing list notes to be lost
Summary: Receiving a serial item causes routing list notes to be lost
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 23416
  Show dependency treegraph
 
Reported: 2014-01-08 01:59 UTC by Liz Rea
Modified: 2020-06-04 20:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 11492: Keep routing notes when receiving next serial (7.17 KB, patch)
2018-12-07 01:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 11492: Keep routing notes when receiving next serial (7.15 KB, patch)
2018-12-07 01:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 11492: Keep routing notes when receiving next serial (7.31 KB, patch)
2018-12-11 12:34 UTC, Devinim
Details | Diff | Splinter Review
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue (2.13 KB, patch)
2018-12-14 01:59 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 11492: Keep routing notes when receiving next serial (7.36 KB, patch)
2019-05-13 17:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue (2.19 KB, patch)
2019-05-13 17:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 11492: Keep routing notes when receiving next serial (7.45 KB, patch)
2019-05-31 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue (2.29 KB, patch)
2019-05-31 07:05 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 Liz Rea 2014-01-08 01:59:27 UTC
To replicate: 

Create a routing list for a subscription
Add some borrowers to it
Add a note
Save it
View the routing list, the note should be appearing

Receive the next serial in the subscription
View the routing list
Note that the routing list notes are now gone. Uh oh.
Comment 1 Liz Rea 2014-01-08 02:47:38 UTC
Further review shows that the notes are stored in the database, but are not displayed properly.

The interface seems to suggest that changing the dropdown would show the list for that particular issue (and it's notes, if it has some). That does not appear to be the case -- for notes, the actual membership of the list I have not checked on (yet). In fact, it looks like it shows the notes for the most recent received serial only, and switching the volume doesn't change the data displayed.
Comment 2 Aleisha Amohia 2018-12-07 01:08:57 UTC
Created attachment 82945 [details] [review]
Bug 11492: Keep routing notes when receiving next serial

This patch ensures the routing notes are carried over when generating
the next serial.

To test:
1) Create a routing list for a subscription
2) Add a borrower and a note to the routing list
3) Generate the next serial (serials-collection.pl)
4) Edit the routing list to see the notes
5) Note that the notes have disappeared
6) Apply patch
7) Edit the routing list, add a note
8) Generate the next serial
9) Edit the routing list and confirm the note is still there
10) Confirm you are still able to edit serials (serials-edit.pl) and
routing notes stay

Sponsored-by: Plant and Food Research Limited
Comment 3 Aleisha Amohia 2018-12-07 01:38:24 UTC
Created attachment 82946 [details] [review]
Bug 11492: Keep routing notes when receiving next serial

This patch ensures the routing notes are carried over when generating
the next serial.

To test:
1) Create a routing list for a subscription
2) Add a borrower and a note to the routing list
3) Generate the next serial (serials-collection.pl)
4) Edit the routing list to see the notes
5) Note that the notes have disappeared
6) Apply patch
7) Edit the routing list, add a note
8) Generate the next serial
9) Edit the routing list and confirm the note is still there
10) Confirm you are still able to edit serials (serials-edit.pl) and
routing notes stay

Sponsored-by: Plant and Food Research Limited
Comment 4 Devinim 2018-12-11 12:34:17 UTC
Created attachment 83036 [details] [review]
Bug 11492: Keep routing notes when receiving next serial

This patch ensures the routing notes are carried over when generating
the next serial.

To test:
1) Create a routing list for a subscription
2) Add a borrower and a note to the routing list
3) Generate the next serial (serials-collection.pl)
4) Edit the routing list to see the notes
5) Note that the notes have disappeared
6) Apply patch
7) Edit the routing list, add a note
8) Generate the next serial
9) Edit the routing list and confirm the note is still there
10) Confirm you are still able to edit serials (serials-edit.pl) and
routing notes stay

Sponsored-by: Plant and Food Research Limited

Signed-off-by: Devinim <kohadevinim@devinim.com.tr>
Comment 5 Jonathan Druart 2018-12-12 19:44:37 UTC
Hi Aleisha,

You are doing things that are not advertised in the commit message. For instance you are passing $publisheddatetext to NewIssue. I feel like this is wrong, $publisheddatetext should not be kept from one issue to another.

There is
1160         my $nextpublisheddate = GetNextDate($subscription, $publisheddate, 1);
1161         my $nextpubdate = $nextpublisheddate;

And $nextpubdate is passed as $planneddate and $publisheddate for the next one, but $publisheddatetext will be unchanged.
Comment 6 Aleisha Amohia 2018-12-14 01:47:34 UTC
(In reply to Jonathan Druart from comment #5)
> Hi Aleisha,
> 
> You are doing things that are not advertised in the commit message. For
> instance you are passing $publisheddatetext to NewIssue. I feel like this is
> wrong, $publisheddatetext should not be kept from one issue to another.
> 
> There is
> 1160         my $nextpublisheddate = GetNextDate($subscription,
> $publisheddate, 1);
> 1161         my $nextpubdate = $nextpublisheddate;
> 
> And $nextpubdate is passed as $planneddate and $publisheddate for the next
> one, but $publisheddatetext will be unchanged.

Hi Jonathan,
I only did this because the subroutine already took $publisheddatetext as a parameter, so thought it would be okay. I will resubmit my patch with an empty string in the place of $publisheddatetext, but otherwise this seems like a different problem.
Comment 7 Aleisha Amohia 2018-12-14 01:59:02 UTC
Created attachment 83192 [details] [review]
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue

I have removed publisheddatetext as a param from my patch, but only in
the places where my patch added it. Any other instance already existed
before my patch.
Comment 8 Liz Rea 2019-05-13 17:31:40 UTC
Created attachment 89675 [details] [review]
Bug 11492: Keep routing notes when receiving next serial

This patch ensures the routing notes are carried over when generating
the next serial.

To test:
1) Create a routing list for a subscription
2) Add a borrower and a note to the routing list
3) Generate the next serial (serials-collection.pl)
4) Edit the routing list to see the notes
5) Note that the notes have disappeared
6) Apply patch
7) Edit the routing list, add a note
8) Generate the next serial
9) Edit the routing list and confirm the note is still there
10) Confirm you are still able to edit serials (serials-edit.pl) and
routing notes stay

Sponsored-by: Plant and Food Research Limited

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Tested on top of 22812 because docker runs in strict mode.
Comment 9 Liz Rea 2019-05-13 17:31:50 UTC
Created attachment 89676 [details] [review]
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue

I have removed publisheddatetext as a param from my patch, but only in
the places where my patch added it. Any other instance already existed
before my patch.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 10 Marcel de Rooy 2019-05-31 06:32:13 UTC
QA: Looking here now
Comment 11 Marcel de Rooy 2019-05-31 07:03:40 UTC
Hmm I'm having the impression that the whole issue field on the edit routing list form is kind of useless. The notes are written to all serial records, and the borrowers are kept on routing list level. Strange design ;)
Comment 12 Marcel de Rooy 2019-05-31 07:05:37 UTC
Created attachment 90214 [details] [review]
Bug 11492: Keep routing notes when receiving next serial

This patch ensures the routing notes are carried over when generating
the next serial.

To test:
1) Create a routing list for a subscription
2) Add a borrower and a note to the routing list
3) Generate the next serial (serials-collection.pl)
4) Edit the routing list to see the notes
5) Note that the notes have disappeared
6) Apply patch
7) Edit the routing list, add a note
8) Generate the next serial
9) Edit the routing list and confirm the note is still there
10) Confirm you are still able to edit serials (serials-edit.pl) and
routing notes stay

Sponsored-by: Plant and Food Research Limited

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2019-05-31 07:05:41 UTC
Created attachment 90215 [details] [review]
Bug 11492: (follow-up) Don't pass publisheddatetext param to NewIssue

I have removed publisheddatetext as a param from my patch, but only in
the places where my patch added it. Any other instance already existed
before my patch.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Martin Renvoize 2019-06-04 09:07:55 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Fridolin Somers 2019-06-10 14:21:00 UTC
Pushed to 19.05.x for 19.05.01
Comment 16 Lucas Gass 2019-06-11 16:14:26 UTC
backported to 18.11.x for 18.11.07