|
Description
Nicolas Legrand
2020-01-13 16:47:29 UTC
Created attachment 97328 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 97329 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 97330 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 101625 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101626 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101627 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 101628 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101629 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101630 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 101721 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101722 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101723 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Small typo :
=head2 ModReserveAffect
- &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id);
+ &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id, $desk);
sub ModReserveAffect {
- my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id ) = @_;
+ my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id, $desk_id ) = @_;
Doc line should say $desk_id instead of $desk right ?
Created attachment 101825 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. (In reply to Fridolin SOMERS from comment #13) > Small typo : > > =head2 ModReserveAffect > > - > &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id); > + > &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id, > $desk); > > sub ModReserveAffect { > - my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id ) = @_; > + my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id, $desk_id > ) = @_; > > Doc line should say $desk_id instead of $desk right ? right, thank you Frido! Created attachment 101884 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101885 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101886 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 101898 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101899 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101900 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Fixed up the schema change and kohastructure change. Created attachment 101955 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 101956 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 101957 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 13881 and then Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. (In reply to Martin Renvoize from comment #22) > Fixed up the schema change and kohastructure change. Now it's really fixed.. doh! Comment on attachment 101956 [details] [review] Bug 24412: kohastructure and atomic update Review of attachment 101956 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_24412_Attach_waiting_reserve_to_desk.perl @@ +1,4 @@ > +$DBversion = 'XXX'; # will be replaced by the RM > +if( CheckVersion( $DBversion ) ) { > + # you can use $dbh here like: > + $dbh->do(q{ there should be check for column existance before adding it, something like: if ( !column_exists( 'reserves', 'desk_id' ) ) { $dbh->do(...); } @@ +5,5 @@ > +ALTER TABLE reserves ADD COLUMN desk_id INT(11) DEFAULT NULL AFTER branchcode, > + ADD KEY desk_id (`desk_id`), > + ADD CONSTRAINT `reserves_ibfk_6` FOREIGN KEY (`desk_id`) REFERENCES `desks` (`desk_id`) ON DELETE CASCADE ON UPDATE CASCADE ; > + }); > + $dbh->do(q{ there should be check for column existance too Comment on attachment 101957 [details] [review] Bug 24412: Attach waiting reserve to desk Review of attachment 101957 [details] [review]: ----------------------------------------------------------------- ::: Koha/Hold.pm @@ +173,3 @@ > }; > > + if (defined $desk_id) { $values->{'desk_id'} = $desk_id }; the desk_id => $desk_id, setting in values above should be omitted, only the one in if should stay @@ +323,5 @@ > + my ($self) = @_; > + > + $self->{_desk} ||= Koha::Desks->find( $self->desk_id() ); > + > + return $self->{_desk}; You should use relation and _new_from_dbic here, see https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL15:_Object-oriented_code_and_the_Koha::_namespace_.5BPROPOSAL_FOR_CHANGE_dev_meeting_2.2F2020.5D ::: Koha/Template/Plugin/Desks.pm @@ +47,5 @@ > + > +sub GetName { > + my ( $self, $desk_id ) = @_; > + my $d = Koha::Desks->find( $desk_id ); > + return (defined $d) ? $d->unblessed->{'desk_name'} : q{}; should be something like: return $d ? $d->desk_name : q{}; ::: koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc @@ +120,4 @@ > [% IF ( hold.found ) %] > [% IF ( hold.atdestination ) %] > [% IF ( hold.found ) %] > + Item waiting at <b> [% hold.wbrname | html %]</b>[% IF hold.desk_name %], [% hold.desk_name | html %],[% END %] <input type="hidden" name="pickup" value="[% hold.wbrcode | html %]" /> since [% hold.waiting_date | $KohaDates %] Looks like you don't need to use Desks template plugin. ::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt @@ +381,4 @@ > [% SET hold = item.first_hold %] > [% IF hold %] > [% IF hold.waitingdate %] > + Waiting at [% Branches.GetName( hold.branchcode ) | html %][% IF ( hold.desk_id ) %], [% Desks.GetName ( hold.desk_id ) | html %][% END %] since [% hold.waitingdate | $KohaDates %]. you don't new Desks template plugin, just use hold.desk.desk_name ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ +468,4 @@ > > <div class="modal-footer"> > <input type="hidden" name="cancel_reserve" value="0" /> > + <button type="submit" class="btn btn-default approve" [% IF (Desks.defined) %]data-dismiss="modal" [% END %]> Will need update as Desks.defined was removed in previous patches on bug 24201 ::: koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc @@ +106,4 @@ > <i class="fa fa-exclamation-circle text-warning"></i> > [% IF ( HOLD.is_at_destination ) %] > [% IF ( HOLD.found ) %] > + Item waiting at <b> [% HOLD.branch.branchname | html %]</b>[% IF ( HOLD.desk_id ) %], [% Desks.GetName ( HOLD.desk_id ) | html %],[% END %] you don't new Desks template plugin, just use hold.desk.desk_name Created attachment 106586 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. This is just a rebase of the failed version :D. Corrected one coming soonishly. Created attachment 106648 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Created attachment 110067 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 110068 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 110069 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Created attachment 110070 [details] [review] Bug 24412: Schema file DO NOT PUSH! Rebased! Comment on attachment 110068 [details] [review] Bug 24412: Attach waiting reserve to desk Review of attachment 110068 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ +462,4 @@ > <input type="hidden" name="cancel_reserve" value="0" /> > <input type="hidden" name="barcode" value="[% barcode | html %]" /> > > + <button type="submit" class="btn btn-default approve" [% IF ( Desks.ListForLibrary ) %]data-dismiss="modal" [% END %]> Could you explain this change.. I'm not sure I understand it's purpose? Ho. I feel uncomfortable. When I saw it I thought “how peculiar... But maybe there is a reason anyway...” And I let it... I feel a bit ashamed. I'm going to revert this. Created attachment 110450 [details] [review] Bug 24412: (follow-up) QA useless change. Maybe it was a relic of something usefull... anyway not anymore. Created attachment 110451 [details] [review] Bug 24412: kohastructure and atomic update Created attachment 110452 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Created attachment 110453 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Created attachment 110454 [details] [review] Bug 24412: Schema file DO NOT PUSH! Created attachment 110455 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Created attachment 110582 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 110583 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 110584 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 110585 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 110586 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works well, Signing off. One minor query, would you ever need to alter which desk the item is located at.. either at return time or via another method? (In reply to Martin Renvoize from comment #50) > Works well, Signing off. ^^ thanks! > One minor query, would you ever need to alter which desk the item is located > at.. either at return time or via another method? I don't understand? The behaviour right now is : if you re-check-in a waiting item with another desk in your session, it changes the desk where the reserved item is waiting. This is not a usecase for us, but I find this useful : it's easy to move the book internally. Unfortunately, while I was looking at this, I found two bugs : (1) if you check in a book waiting at another library, it makes a server internal error. (2) When changing libraries, the menu proposes only one desk and not the entire set. I'll look at (1) here and (2) in a separate bugzilla... (In reply to Nicolas Legrand from comment #51) > (In reply to Martin Renvoize from comment #50) > > Works well, Signing off. > > ^^ thanks! > > > One minor query, would you ever need to alter which desk the item is located > > at.. either at return time or via another method? > > I don't understand? I hadn't thought about a second checkin, but I'm glad that works as expected. I suppose I was wondering about whether there was a need to expose the desks as a selectable option at checkin should you want to pass the book to another desk there and then.. but actually I'm not sure there's really a use case for that. > Unfortunately, while I was looking at this, I found two bugs : (1) if you > check in a book waiting at another library, it makes a server internal > error. (2) When changing libraries, the menu proposes only one desk and not > the entire set. > > I'll look at (1) here and (2) in a separate bugzilla... Good catches [U+1F60A] > > Unfortunately, while I was looking at this, I found two bugs : (1) if you
> > check in a book waiting at another library, it makes a server internal
> > error. (2) When changing libraries, the menu proposes only one desk and not
> > the entire set.
> >
> > I'll look at (1) here and (2) in a separate bugzilla...
>
> Good catches [U+1F60A]
Awaiting the follow-up!
I am also missing tests? Created attachment 112046 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112047 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112048 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112049 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112050 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> this was a big rebase I didn't test (the Koha people worked a lot while I was in my tunnel!). I'll test tomorrow so please don't test until I switch to NEED SIGNOFF :). Thanks! Created attachment 112092 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Created attachment 112099 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 6 -v 2 2. should be green (In reply to Katrin Fischer from comment #54) > I am also missing tests? oups... Tests added! > Unfortunately, while I was looking at this, I found two bugs : (1) if you > check in a book waiting at another library, it makes a server internal > error. (2) When changing libraries, the menu proposes only one desk and not > the entire set. > > I'll look at (1) here and (2) in a separate bugzilla... I forgot to mention it seems like (1) was fixed by Bug 12556! > > Unfortunately, while I was looking at this, I found two bugs : (1) if you > check in a book waiting at another library, it makes a server internal > error. (2) When changing libraries, the menu proposes only one desk and not > the entire set. > > I'll look at (1) here and (2) in a separate bugzilla... fix proposed for (2) in Bug 26779 Séverine spotted bugs. Created attachment 112165 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=26779 Created attachment 112166 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112167 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112168 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112169 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112170 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Created attachment 112171 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 7 -v 2 2. should be green Created attachment 112172 [details] [review] Bug 24412: (follow-up) bad desk printing forgot to update two pages when I wrote Hold->desk to replace Desks.GetName. Created attachment 112201 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Created attachment 112202 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112203 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112204 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112205 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112206 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Created attachment 112207 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 8 -v 2 2. should be green Created attachment 112208 [details] [review] Bug 24412: (follow-up) bad desk printing forgot to update two pages when I wrote Hold->desk to replace Desks.GetName. Created attachment 112209 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Created attachment 112210 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Created attachment 112211 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=26779 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112212 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112213 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112214 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112215 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112216 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112217 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 8 -v 2 2. should be green Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112218 [details] [review] Bug 24412: (follow-up) bad desk printing forgot to update two pages when I wrote Hold->desk to replace Desks.GetName. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112219 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 112264 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112265 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112266 [details] [review] Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112267 [details] [review] Bug 24412: Schema file DO NOT PUSH! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112268 [details] [review] Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112269 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112270 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 8 -v 2 2. should be green Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112271 [details] [review] Bug 24412: (follow-up) bad desk printing forgot to update two pages when I wrote Hold->desk to replace Desks.GetName. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 112272 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I'm pretty much ready to PQA this.. once I have an answer to a couple of minor queries. 1/ Are you sure you want to delete a reserve that's attached to a desk when the desk is deleted.. rather than setting the desk_id to null? This may need a tiny followup ;) 2/ Would there ever be a requirement for desks to be 'required'.. in which case we should probably introduce a pref and check to enforce this. This can be a separate bug. 3/ Would there ever be a case where the librarian may want to override the desk at the time of setting the reserve as waiting? This could also be in a followup bug. 4/ I'm contemplating asking for a check in the set_waiting that ensures we set a desk that corresponds to the branchcode to ensure we don't get mixed data. I'm generally really happy with this development though, it's looking great. Good Job! Hmmm... also beware that bug 12556 is PQA so this will likely need a rebase on top of it before it gets pushed :| (In reply to Martin Renvoize from comment #104) > Hmmm... also beware that bug 12556 is PQA so this will likely need a rebase > on top of it before it gets pushed :| Ignore that.. 12556 is already in master.. and this applies fine Created attachment 112292 [details] [review] Bug 24412: (QA follow-up) Change CASCADE DELETE to SET NULL I don't believe waiting reserves should be removed if a desk is deleted, we should instead simply unset the desk. Created attachment 112293 [details] [review] Bug 24412: (QA follow-up) DBIC Schema Update (In reply to Martin Renvoize from comment #103) > I'm pretty much ready to PQA this.. once I have an answer to a couple of > minor queries. > > 1/ Are you sure you want to delete a reserve that's attached to a desk when > the desk is deleted.. rather than setting the desk_id to null? This may need > a tiny followup ;) I resolved this in my own QA follow-up > > 2/ Would there ever be a requirement for desks to be 'required'.. in which > case we should probably introduce a pref and check to enforce this. This can > be a separate bug. I decided this wouldn't be the case. We need to allow AddReserve to continue passing through no desk for the 'Automatically set holds to waiting' option of 'ReservesNeedReturns'. It might be nice to have the option of using 'Processing' for that case in the future however. > > 3/ Would there ever be a case where the librarian may want to override the > desk at the time of setting the reserve as waiting? This could also be in a > followup bug. Another one for another bug in the future. > 4/ I'm contemplating asking for a check in the set_waiting that ensures we > set a desk that corresponds to the branchcode to ensure we don't get mixed > data. This is over the top at this time I think. After all that.. I'm Passing QA! 1. POD for Koha::Hold->desk is wrong
2. If not desk set, biblio hold list (reserves/request.pl) explodes with:
Can't call method "desk_name" on an undefined value at /kohadevbox/koha/reserve/request.pl line 661
3. The desk's name is not correctly escaped in the patron's hold list (circ/circulation.pl, "Holds" tab)
Try to use the following string as a desk name:
<script>alert("❤");</script>
Search for "escapeHtml" in the same js file for the solution.
4. I am not sure about the desk_id parameter we passed to Koha::Hold->set_waiting.
We could also set it from ModReserveAffect, what's the benefit of this approach?
Failing QA for 1, 2, 3.
4 is for discussion.
(In reply to Jonathan Druart from comment #109) > 4. I am not sure about the desk_id parameter we passed to > Koha::Hold->set_waiting. > We could also set it from ModReserveAffect, what's the benefit of this > approach? I felt it was a good thing having it at the set_waiting level. In part because it encourages code to be written cleanly in the Koha:: space and because I feel it's the set_waiting call that links closely with when a desk would be assigned to a reserve and as such having it in this function now means if we use set_waiting down the line without calling ModReserveAffect then we would be reminded to set desk_id if appropriate. Created attachment 113108 [details] [review] Bug 24412 (follow-up): Fix POD Koha::Desk and not Koha::Library... Created attachment 113109 [details] [review] Bug 24412: (follow-up) prevent request.pl from failing When no desk is defined request.pl returns an internal server error. Check a desk is defined before asking for reservation deskname. Plan test: 1. log in with a library with no desk 2. check in a reserved book so it'll be switch to “waiting reserve” 3. go to the book notice and click the Holds tab (request.pl page) 4. boum 5. apply patch, restart plack 6. refresh page 7. now loading properly Created attachment 113116 [details] [review] Bug 24412: (follow-up) prevent js injection Some js variables are not properly escaped and can be executed if containing javascript. 1. have some waiting reserve attached to a desk 2. change this desk name to : <script>alert("❤");</script> 3. go to user's checkout page (circulation.pl) and click on the Hold(s) tab 4. you should see some popup with a ❤ in it. 5. apply patch and refresh page 6. now you should see the desk name printed properly in the page: <script>alert("❤");</script> (In reply to Jonathan Druart from comment #109) [...] > > 4. I am not sure about the desk_id parameter we passed to > Koha::Hold->set_waiting. > We could also set it from ModReserveAffect, what's the benefit of this > approach? > > Failing QA for 1, 2, 3. I tried to address 1, 2, 3, a big thanks for spotting those. Especially 3... > 4 is for discussion. I'm a bit ashamed to say I don't really know. It seemed to me logical at the time. I'd say that if you think the day someone rewrite ModReserveAffect as a Koha::Hold method the desk attachment should go in this method, then I'll put it under set_waiting(). Thanks Nicolas! Created attachment 113228 [details] [review] Bug 24412: kohastructure and atomic update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 113229 [details] [review] Bug 24412: Attach waiting reserve to desk When an item is checked in and marked 'Waiting' or already 'Waiting' and there is a desk attached to the session, the item is marked waiting at the current desk of the current library. The information is displayed on the OPAC and on the intranet. The patron can then know at which desk he can retrieve his document. Desk Management (Bug 13881) is now useful. Test plan : 1. apply Bug 24201 2. $KOHA_PATH/installer/data/mysql/updatedatabase.pl 3. Check out some document to someone 4. make another one reserve this document 5. check in the document 6. you can see the document is attach to the current library 7. create some desks and attach one to your session (see Bug 13881 and Bug 24201) 8. cancel the preceding reserve and redo steps 3 to 5 9. you should see the document is waiting at the current library and current desk on: a. the intranet document request page b. the intranet borrower holds tab c. the item list where the document is listed on the bibliographic details d. the borrower's OPAC holds tab. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 24412: (follow-up) QA Following Josef Moravec QA comments : - rewrite Koha::Hold->desk according to Object Oriented Koha Guidelines and use it to fetch desk name in various templates - remove unused Desks.GetName - Check for columns existence in db update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 24412: (follow-up) QA: useless change Maybe it was a relic of something usefull... anyway not anymore. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 24412: (follow-up) Fix POD Koha::Desk and not Koha::Library... Created attachment 113230 [details] [review] Bug 24412: (follow-up) tests Add some test for Koha::Hold->desk and Koha::Hold->set_waiting. Test plan: 1. prove t/db_dependent/Koha/Holds.t 2. should be green! Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 113231 [details] [review] Bug 24412: (follow-up) qa-tools fix obvious problems: 1. qa -c 8 -v 2 2. should be green Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 113232 [details] [review] Bug 24412: (follow-up) bad desk printing forgot to update two pages when I wrote Hold->desk to replace Desks.GetName. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 113233 [details] [review] Bug 24412: (follow-up) prevent error in svc/holds don't fail if a hold has no desk attached. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 113234 [details] [review] Bug 24412: (QA follow-up) Change CASCADE DELETE to SET NULL I don't believe waiting reserves should be removed if a desk is deleted, we should instead simply unset the desk. Created attachment 113235 [details] [review] Bug 24412: (follow-up) prevent request.pl from failing When no desk is defined request.pl returns an internal server error. Check a desk is defined before asking for reservation deskname. Plan test: 1. log in with a library with no desk 2. check in a reserved book so it'll be switch to “waiting reserve” 3. go to the book notice and click the Holds tab (request.pl page) 4. boum 5. apply patch, restart plack 6. refresh page 7. now loading properly Created attachment 113236 [details] [review] Bug 24412: (follow-up) prevent js injection Some js variables are not properly escaped and can be executed if containing javascript. 1. have some waiting reserve attached to a desk 2. change this desk name to : <script>alert("❤");</script> 3. go to user's checkout page (circulation.pl) and click on the Hold(s) tab 4. you should see some popup with a ❤ in it. 5. apply patch and refresh page 6. now you should see the desk name printed properly in the page: <script>alert("❤");</script> I've squashed some patches. Pushed to master for 20.11, thanks to everybody involved! Thanks a lot everyone! Thank you Séverine for testing, thank you, Martin, Josef and Jonathan for reviewing, questioning, advising and guiding. It's such a pleasure to share time with the Koha community! I'm so eager to meet you all in Marseille as soon as possible! See you all! enhancement will not be backported to 20.05.x |