Bug 28377 - Use the API to suspend/resume holds
Summary: Use the API to suspend/resume holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 22206 24850 28937
Blocks: 28521
  Show dependency treegraph
 
Reported: 2021-05-18 19:59 UTC by Tomás Cohen Arazi
Modified: 2021-12-17 15:05 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement changes the patron page (detail and circulation) so that is uses the API to suspend and resume holds on the holds tab. It also removes the svc/hold/{resume|suspend} files as they are no longer used.
Version(s) released in:
22.05.00


Attachments
Bug 28377: Use the API to suspend/resume holds (7.60 KB, patch)
2021-05-18 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend} (4.67 KB, patch)
2021-05-18 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: Use the API to suspend/resume holds (7.54 KB, patch)
2021-09-20 11:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend} (4.66 KB, patch)
2021-09-20 11:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: Use the API to suspend/resume holds (7.59 KB, patch)
2021-09-20 12:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend} (4.71 KB, patch)
2021-09-20 12:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 28377: Use the API to suspend/resume holds (7.66 KB, patch)
2021-11-03 15:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend} (4.75 KB, patch)
2021-11-03 15:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (28937 follow-up) Flatpickr change (1.69 KB, patch)
2021-12-09 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: (QA follow-up) Fallback error handling (1.98 KB, patch)
2021-12-09 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: Use the API to suspend/resume holds (7.70 KB, patch)
2021-12-10 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend} (4.75 KB, patch)
2021-12-10 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (28937 follow-up) Flatpickr change (1.75 KB, patch)
2021-12-10 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (QA follow-up) Fallback error handling (2.04 KB, patch)
2021-12-10 15:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28377: (QA follow-up) .js files should use __() (2.25 KB, patch)
2021-12-13 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28377: (QA follow-up) .js files should use __() (1.69 KB, patch)
2021-12-13 13:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-05-18 19:59:09 UTC
We have routes for suspending and resuming holds. We can deprecate the svc/holds/* scripts.
Comment 1 Tomás Cohen Arazi 2021-05-18 20:52:19 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2021-05-18 20:52:24 UTC Comment hidden (obsolete)
Comment 3 David Nind 2021-09-19 20:59:38 UTC
Patch no longer applies 8-(..

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28377: Use the API to suspend/resume holds
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
M	koha-tmpl/intranet-tmpl/prog/js/holds.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/holds.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28377: Use the API to suspend/resume holds
Comment 4 Tomás Cohen Arazi 2021-09-20 11:22:16 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2021-09-20 11:22:20 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2021-09-20 11:23:01 UTC
(In reply to David Nind from comment #3)
> Patch no longer applies 8-(..

Thanks for your time! Rebased!
Comment 7 David Nind 2021-09-20 12:14:36 UTC
Created attachment 125057 [details] [review]
Bug 28377: Use the API to suspend/resume holds

This patch makes the patron page (detail and circulation) use the API to
suspend/resume holds on the holds tab.

It previously used the old svc/ scripts we plan to replace.

To test
1. Have a patron with some holds
2. Play with suspending/resuming holds. Include the indefinite
   suspension.
=> SUCCESS: Everything works as usual
3. Apply this patch
4. Repeat 2
=> SUCCESS: Nothing changed, a soft breeze surprises you
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2021-09-20 12:14:42 UTC
Created attachment 125058 [details] [review]
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend}

This files are no longer used and thus removed. Their functionality is
already present on the API

POST   /holds/:hold_id/suspension
DELETE /holds/:hold_id/suspension

To test:
1. Apply this patch
2. Run:
   $ git grep 'hold/suspend'
   $ git grep 'hold/resume'
=> SUCCESS: No references to the scripts
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize 2021-09-21 14:10:17 UTC
Given this patchset includes getting and settings dates via the API I believe it will fall short on the same grounds that bug 24608 is currently stuck.

We need to properly handle RFC dates in dt_from_string before I'd be comfortable with us moving to using the API for date setting.
Comment 10 Katrin Fischer 2021-10-22 10:54:05 UTC
(In reply to Martin Renvoize from comment #9)
> Given this patchset includes getting and settings dates via the API I
> believe it will fall short on the same grounds that bug 24608 is currently
> stuck.
> 
> We need to properly handle RFC dates in dt_from_string before I'd be
> comfortable with us moving to using the API for date setting.

Is there a bug for RFC dates and should this be moved to BLOCKED? I'd not like to do so with a clear path forward (bug dependencies) tbh.
Comment 11 Katrin Fischer 2021-10-22 12:40:14 UTC
Verified the bug in question that needs resolving first would be bug 24850.
Comment 12 Martin Renvoize 2021-11-03 15:42:14 UTC
Created attachment 127270 [details] [review]
Bug 28377: Use the API to suspend/resume holds

This patch makes the patron page (detail and circulation) use the API to
suspend/resume holds on the holds tab.

It previously used the old svc/ scripts we plan to replace.

To test
1. Have a patron with some holds
2. Play with suspending/resuming holds. Include the indefinite
   suspension.
=> SUCCESS: Everything works as usual
3. Apply this patch
4. Repeat 2
=> SUCCESS: Nothing changed, a soft breeze surprises you
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-11-03 15:42:19 UTC
Created attachment 127271 [details] [review]
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend}

This files are no longer used and thus removed. Their functionality is
already present on the API

POST   /holds/:hold_id/suspension
DELETE /holds/:hold_id/suspension

To test:
1. Apply this patch
2. Run:
   $ git grep 'hold/suspend'
   $ git grep 'hold/resume'
=> SUCCESS: No references to the scripts
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-11-03 15:43:14 UTC
OK, great work.. all works as expected :).

Passing QA.. I thought it OK to remove the /svc scripts without a deprecation warning as in theory /svc was always a private api.
Comment 15 Jonathan Druart 2021-11-08 10:17:58 UTC
"Suspend until" is not saved.

+        var suspend_until_date = $("#suspend-modal-until").datepicker("getDate");

We are not using flatpickr
Comment 16 Jonathan Druart 2021-11-08 10:18:28 UTC
+                        if (jqXHR.status === 404) {

What about other errors? We shouldn't ignore them IMO.
Comment 17 Tomás Cohen Arazi 2021-12-06 11:18:08 UTC
(In reply to Jonathan Druart from comment #15)
> "Suspend until" is not saved.
> 
> +        var suspend_until_date =
> $("#suspend-modal-until").datepicker("getDate");
> 
> We are not using flatpickr

Right, it was migrated after the patches were tested.

> +                        if (jqXHR.status === 404) {
> 
> What about other errors? We shouldn't ignore them IMO.

I'm not sure. I mean, it is doing the exact same thing it did before. Would you agree with a fallback for the rest of the errors like 'Your request could not be processed. Check the logs' kind of thing?
Comment 18 Jonathan Druart 2021-12-06 13:05:11 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> (In reply to Jonathan Druart from comment #15)
> > +                        if (jqXHR.status === 404) {
> > 
> > What about other errors? We shouldn't ignore them IMO.
> 
> I'm not sure. I mean, it is doing the exact same thing it did before. Would
> you agree with a fallback for the rest of the errors like 'Your request
> could not be processed. Check the logs' kind of thing?

Yes.
Comment 19 Tomás Cohen Arazi 2021-12-06 13:06:41 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Tomás Cohen Arazi from comment #17)
> > (In reply to Jonathan Druart from comment #15)
> > > +                        if (jqXHR.status === 404) {
> > > 
> > > What about other errors? We shouldn't ignore them IMO.
> > 
> > I'm not sure. I mean, it is doing the exact same thing it did before. Would
> > you agree with a fallback for the rest of the errors like 'Your request
> > could not be processed. Check the logs' kind of thing?
> 
> Yes.

Thanks.
Comment 20 Tomás Cohen Arazi 2021-12-09 19:23:52 UTC
Created attachment 128380 [details] [review]
Bug 28377: (28937 follow-up) Flatpickr change

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2021-12-09 19:40:32 UTC
Created attachment 128381 [details] [review]
Bug 28377: (QA follow-up) Fallback error handling

The code only dealt with 404, this patch makes it have a fallback
message for failure codes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Martin Renvoize 2021-12-10 15:57:17 UTC
Created attachment 128402 [details] [review]
Bug 28377: Use the API to suspend/resume holds

This patch makes the patron page (detail and circulation) use the API to
suspend/resume holds on the holds tab.

It previously used the old svc/ scripts we plan to replace.

To test
1. Have a patron with some holds
2. Play with suspending/resuming holds. Include the indefinite
   suspension.
=> SUCCESS: Everything works as usual
3. Apply this patch
4. Repeat 2
=> SUCCESS: Nothing changed, a soft breeze surprises you
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2021-12-10 15:57:21 UTC
Created attachment 128403 [details] [review]
Bug 28377: (follow-up) Remove svc/hold/{resume|suspend}

This files are no longer used and thus removed. Their functionality is
already present on the API

POST   /holds/:hold_id/suspension
DELETE /holds/:hold_id/suspension

To test:
1. Apply this patch
2. Run:
   $ git grep 'hold/suspend'
   $ git grep 'hold/resume'
=> SUCCESS: No references to the scripts
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2021-12-10 15:57:25 UTC
Created attachment 128404 [details] [review]
Bug 28377: (28937 follow-up) Flatpickr change

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2021-12-10 15:57:29 UTC
Created attachment 128405 [details] [review]
Bug 28377: (QA follow-up) Fallback error handling

The code only dealt with 404, this patch makes it have a fallback
message for failure codes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2021-12-10 15:58:05 UTC
Retested, back to PQA
Comment 27 Fridolin Somers 2021-12-11 06:49:51 UTC
+ alert(_("Your request could not be processed. Check the logs"));
It is in holds.js, shouldn't it use __("foo") ?
Comment 28 Katrin Fischer 2021-12-11 12:42:28 UTC
(In reply to Fridolin Somers from comment #27)
> + alert(_("Your request could not be processed. Check the logs"));
> It is in holds.js, shouldn't it use __("foo") ?

Yes, good catch!
Comment 29 Tomás Cohen Arazi 2021-12-13 13:20:08 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2021-12-13 13:21:28 UTC
(In reply to Fridolin Somers from comment #27)
> + alert(_("Your request could not be processed. Check the logs"));
> It is in holds.js, shouldn't it use __("foo") ?

Thanks for spotting that. I was really lost in the translation arena until you pointed this here.
Comment 31 Jonathan Druart 2021-12-13 13:29:24 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> There was another occurence from prior patches (mine :-/) that gets
> fixed as well.

Should be on its own bug report if it's in stable.
Comment 32 Tomás Cohen Arazi 2021-12-13 13:37:09 UTC
Created attachment 128461 [details] [review]
Bug 28377: (QA follow-up) .js files should use __()

As noted by QA, this bug contained uses of _() which is not correct in
include .js files [1].

[1] https://wiki.koha-community.org/wiki/Coding_Guidelines#JS5:_Enabling_translation_of_language_strings_in_linked_JavaScript

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2021-12-13 13:37:50 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Tomás Cohen Arazi from comment #29)
> > There was another occurence from prior patches (mine :-/) that gets
> > fixed as well.
> 
> Should be on its own bug report if it's in stable.

Right, removed from here, submitting somewhere else.
Comment 34 Fridolin Somers 2021-12-16 22:13:20 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄