Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for hold start date and hold modification #1956

Closed
wants to merge 5 commits into from

Conversation

EreMaijala
Copy link
Contributor

(N.B. A set of separate but related commits)

This set of changes adds the following:

  • Support for specifying start date for a hold
  • Validation of dates before sending them to the ILS driver
    • There's some variation on how things are handled in each driver, and this allows them to be simplified.
  • Modification of holds

Notes:

  • The markup for the Select all checkbox in holds.phtml is a bit odd, but it's the same as in the checkouts page.
  • Form handling is a bit complicated due to the no-js support for canceling requests.
  • Update won't work without JS, so the button is only displayed via JS.
  • Creation of $safeId is changed to work properly when there are multiple holds on a single bibliographic record.
  • The date handling cleanup is not yet reflected in ILS drivers.
  • "Start date" and "Frozen until" are mostly the same at least for the drivers involved at the moment, but they're handled as different ones based on what's logical.

TODO:

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have time for an especially thorough review, and I haven't actually tested anything yet, but here are a few initial comments based on a fairly cursory examination of the code.

languages/en.ini Outdated
@@ -514,6 +514,15 @@ hold_cancel_success = "Your request was successfully canceled"
hold_cancel_success_items = "%%count%% request(s) were successfully canceled"
hold_date_invalid = "Please enter a valid date"
hold_date_past = "Please enter a date in the future"
hold_edit_failed_items = "Updating failed for %%count%% hold(s)"
hold_edit_frozen = "Frozen Status"
hold_edit_frozen_set = "Freeze (inactivate)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if "suspend" or "temporarily disable" would be better wording than "inactivate."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, ok?

config/vufind/KohaRest.ini Show resolved Hide resolved
module/VuFind/src/VuFind/Controller/HoldsTrait.php Outdated Show resolved Hide resolved
* @return mixed
*/
public function editHoldsAction()
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an enormous amount of code here... given that the MyResearchController is already unreasonably huge, I'd love to avoid making it dramatically larger still. Maybe refactoring MyResearchController is a project for another day and we should just live with this for now, but if there's a logical way to move more code to a helper or support class, that might save us time down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried initially to put this in the holds plugin like cancelHolds is done, but interfacing it with the action method became too complicated. I suppose something like separate controllers for different parts of MyResearch (like LibraryCardsController now) would be a more natural way of splitting things up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have been wanting to split up MyResearchController for years, but I just never find the time since it's not broken, it's just ugly. Ideally, at the same time we split this up, we could also switch to dependency-injected controllers to move away from explicit reliance on the service manager (as discussed in VUFIND-966). So many improvement projects, so little time! Unfortunately, I suspect the increasing urgency of replacing laminas-db is going to take up a lot of my time before I get around to working on this important but less-vital goal.

In any case, getting back down to the task at hand, if it would make sense to establish a HoldsController and start splitting some functionality away now, maybe that would offer an excuse to begin some of the necessary refactoring... but if that makes the rest of this project harder, we can just leave it as-is for now and deal with the refactoring later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing a HoldsController now would make sense. I'll just do that and move HoldsAction there as well. Do you have a preference on whether to keep the holds.phtml template in the current location for back-compatibility? I'm tempted to move it as well to make its location standard for the future, but that'd be an incompatible change. I think that keeping the old route is necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to moving holds.phtml as long as we document it in the changelog. Would it make sense to move the existing Holds functionality as a separate PR and then merge that work here, to keep things broken into smaller chunks? If that makes the work harder, though, I'm fine with having it done here -- we'll just want to add a TODO checkbox about the changelog so I don't lose track of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost put it here, but decided it's so much cleaner to do the refactoring separately and posted PR #1961. Now I just need it in to continue here...

themes/bootstrap3/js/lightbox.js Show resolved Hide resolved
@EreMaijala
Copy link
Contributor Author

To make things easier to manage, I'll close this one and post separate PR's for start date + validation and hold modification when #1960 and #1961 are done.

@EreMaijala EreMaijala closed this May 20, 2021
@EreMaijala EreMaijala deleted the hold-improvements branch September 15, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants