|
Description
Tomás Cohen Arazi (tcohen)
2021-05-18 19:59:09 UTC
Created attachment 121129 [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 Created attachment 121130 [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> 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 Created attachment 125052 [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 Created attachment 125053 [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> (In reply to David Nind from comment #3) > Patch no longer applies 8-(.. Thanks for your time! Rebased! 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> 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> 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. (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. Verified the bug in question that needs resolving first would be bug 24850. 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> 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> 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. "Suspend until" is not saved.
+ var suspend_until_date = $("#suspend-modal-until").datepicker("getDate");
We are not using flatpickr
+ if (jqXHR.status === 404) {
What about other errors? We shouldn't ignore them IMO.
(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? (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. (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. Created attachment 128380 [details] [review] Bug 28377: (28937 follow-up) Flatpickr change Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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> 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> Retested, back to PQA + alert(_("Your request could not be processed. Check the logs"));
It is in holds.js, shouldn't it use __("foo") ?
(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! Created attachment 128458 [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]. There was another occurence from prior patches (mine :-/) that gets fixed as well. [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> (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. (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. 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> (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. Pushed to master for 22.05, thanks to everybody involved [U+1F984] |