fix for new hold form for non holdable records #240

banukutlu · 2020-08-31T18:29:17Z

Issue #237, also fixes Issue #220

…to summaries

cdmo · 2020-09-03T01:35:50Z

reopened to add a message Sorry! You cannot place a hold on this item. Contact your library if you need assistance for records that are not holdable eg. catkey: 31200756

I just ran http://localhost:3001/holds/new?catkey=31200756 and it did not work but I got the following:

I was expecting to get "Sorry! You cannot place a hold on this item. Contact your library if you need assistance"

banukutlu · 2020-09-03T03:08:03Z

from issue

reopened to add a message Sorry! You cannot place a hold on this item. Contact your library if you need assistance for records that are not holdable eg. catkey: 31200756

I just ran http://localhost:3001/holds/new?catkey=31200756 and it did not work but I got the following:

I was expecting to get "Sorry! You cannot place a hold on this item. Contact your library if you need assistance"

good catch! this is an interesting one, it is an online resource but has bibinfo and callList. Wonder what the criteria should be to determine if it is holdable or not for such records 🤔 Currently I'm checking if callList is blank or not but that check is not enough for this record.

ruthtillman · 2020-09-04T18:25:58Z

While nice to fix if we get a handle on criteria, this is not an issue we need to spend time on. The failure message as screenshotted is sufficient.

cdmo · 2020-09-08T01:36:41Z

Can you give me the catkeys of records you tested with so I can check them out? The reason I caught 31200756 is because it was the one mentioned in the issue. Thanks!

banukutlu · 2020-09-08T13:59:49Z

Can you give me the catkeys of records you tested with so I can check them out? The reason I caught 31200756 is because it was the one mentioned in the issue. Thanks!

There were couple more that I could not find out right now, will add here if I do:

19200019
31200756
a non-existing key
no key (just going to holds/new or holds/new?catkey=)

ruthtillman · 2020-09-08T14:00:37Z

I also used

2308333

ruthtillman · 2020-09-08T14:01:55Z

Yields error: To place a hold, please select a volume. 17735134

banukutlu · 2020-09-08T16:38:09Z

Yields error: To place a hold, please select a volume. 17735134

It has a callList but no items with holdable locations so it cannot find a barcode. I think for this it should not load the new form and displays that message, right? I will add a fix.

ruthtillman · 2020-09-08T16:59:05Z

Yes, ideally it wouldn't load the form.

… msg

app/controllers/holds_controller.rb

cdmo

19200019 has holdable items and I was able to add a request for it just fine.
31200756 (a record that just points to a free and open access resource) produced the error "Hold not allowed".
a non-existing key produced a Rails flash error of "Sorry! You cannot place a hold on this item. Contact your library if you need assistance."
no catkey provided produced a Rails flash error of "Please select an item to place a hold."
2308333 (unholdable item, needs to be requested through special form) produced "Hold not allowed" like the OA link
177351341 (thesis) indeed now does not load the form and instead produces a Rails flash error of "Sorry! You cannot place a hold on this item. Contact your library if you need assistance."

So...

Seems like we should maybe use the "Sorry you can't place a hold on this item!" message before allowing the patron to fill out the new hold form for OA links (31200756) and special request items (2308333) too. Maybe all unholdable items should have this same setup?
The error message "Please select an item to place a hold." for when no catkey is specified, I wonder if we should change the language? Because when you get that error you don't have an option to "select" any item to place a hold. It's just kinda confusing to me. I sorta wonder if we even need this at all. I can't imagine this scenario ever happening in the real use of the application. If we want to keep it maybe just make a general error message like "There was a problem processing your hold request, please contact your library if you need assistance." 🤷‍♂️

ruthtillman · 2020-09-09T13:37:16Z

Nice to have. Consistency would be ideal. I am ok with launching before that part is consistent because neither of those has an "I Want It" button and therefore we're dealing with an edge case that is already being handled by suppressing "I Want It." So we can prioritize other things before addressing thing.
Yes, change to "There was a problem processing your hold request, please contact your library if you need assistance." Ideally they won't run into that either, but if they are we should also know about it and learn how they got there.

cdmo

Ok, so we want to update the language on the no catkey provided error

config/locales/en.yml

app/services/place_hold_form/builder.rb

Co-authored-by: Charlie Morris <523381+cdmo@users.noreply.github.com>

cdmo

As per our zoom, move filter_holdables invocation into the generate method and add a blank? check on call_list after (just a single blank? check) oh and maybe return an empty hash instead of implicit nil? :)

it is irritating that Bib can have a @record of just an error. Like it should not be a Bib it should be a SirsiError or something. Setting this aside.

cdmo · 2020-09-11T16:43:36Z

app/services/place_hold_form/builder.rb

@@ -95,4 +102,8 @@ def naturalize(value)
    def volumetric?
      @call_list&.any? { |call| call.volumetric.present? }
    end
+
+    def find_barcode


cdmo

Overall I think we're good, I'm pushing one commit with some semantics suggestions and code organizing suggestions - can you review?

banukutlu · 2020-09-14T15:13:23Z

app/services/place_hold_form/builder.rb

-
-    # no holdable items found
-    return {} if @call_list.blank?
+    return {} unless holdables_present?(bib_info)


liked this 👌 good to have the generate method shorter as a summary

spec/services/place_hold_form/builder_spec.rb

banukutlu · 2020-09-14T15:15:47Z

Overall I think we're good, I'm pushing one commit with some semantics suggestions and code organizing suggestions - can you review?

Your refactor looks good 👌 thanks!

cdmo

Teamwork!

banukutlu · 2020-09-14T16:36:55Z

Teamwork!

🍻

banukutlu added this to the 0.3.x Theming and Final Prep milestone Aug 31, 2020

banukutlu requested a review from cdmo August 31, 2020 18:29

banukutlu self-assigned this Aug 31, 2020

adds a message for new hold form if record is not holdable, redirect …

cd899f0

…to summaries

banukutlu force-pushed the 237-fix-new-hold-form-for-no-holdable-locs branch from 6fe2c8e to cd899f0 Compare August 31, 2020 18:30

banukutlu mentioned this pull request Sep 2, 2020

50x template did not appear #220

Closed

when catkey param missing

07e61f9

record with no holdable locations should redirect to summaries with a…

9b0a1a6

… msg

cdmo reviewed Sep 9, 2020

View reviewed changes

app/controllers/holds_controller.rb Show resolved Hide resolved

cdmo reviewed Sep 9, 2020

View reviewed changes

cdmo suggested changes Sep 9, 2020

View reviewed changes

Banu Kutlu added 2 commits September 9, 2020 13:10

updating language on the missing param catkey msg

a1a92ff

updating specs as well

7321279

banukutlu requested a review from cdmo September 9, 2020 17:26

cdmo reviewed Sep 10, 2020

View reviewed changes

config/locales/en.yml Outdated Show resolved Hide resolved

cdmo reviewed Sep 10, 2020

View reviewed changes

app/services/place_hold_form/builder.rb Outdated Show resolved Hide resolved

Update config/locales/en.yml

020ed19

Co-authored-by: Charlie Morris <523381+cdmo@users.noreply.github.com>

cdmo suggested changes Sep 10, 2020

View reviewed changes

Banu Kutlu added 2 commits September 10, 2020 14:59

refactor

ea6dc5d

refactor to make CC happy

6a8c605

cdmo reviewed Sep 11, 2020

View reviewed changes

Suggesting we reorganize all that holdable and call_list checking a bit

a56a570

cdmo suggested changes Sep 11, 2020

View reviewed changes

banukutlu commented Sep 14, 2020

View reviewed changes

spec/services/place_hold_form/builder_spec.rb Show resolved Hide resolved

banukutlu requested a review from cdmo September 14, 2020 15:15

cdmo approved these changes Sep 14, 2020

View reviewed changes

cdmo merged commit 47b9dbb into master Sep 14, 2020

cdmo deleted the 237-fix-new-hold-form-for-no-holdable-locs branch September 14, 2020 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for new hold form for non holdable records #240

fix for new hold form for non holdable records #240

banukutlu commented Aug 31, 2020 •

edited

Loading

cdmo commented Sep 3, 2020

banukutlu commented Sep 3, 2020 •

edited

Loading

ruthtillman commented Sep 4, 2020

cdmo commented Sep 8, 2020

banukutlu commented Sep 8, 2020 •

edited

Loading

ruthtillman commented Sep 8, 2020

ruthtillman commented Sep 8, 2020

banukutlu commented Sep 8, 2020 •

edited

Loading

ruthtillman commented Sep 8, 2020

cdmo left a comment •

edited

Loading

ruthtillman commented Sep 9, 2020

cdmo left a comment

cdmo left a comment •

edited

Loading

cdmo Sep 11, 2020

cdmo left a comment

banukutlu Sep 14, 2020

cdmo Sep 14, 2020

banukutlu commented Sep 14, 2020

cdmo left a comment

banukutlu commented Sep 14, 2020

fix for new hold form for non holdable records #240

fix for new hold form for non holdable records #240

Conversation

banukutlu commented Aug 31, 2020 • edited Loading

cdmo commented Sep 3, 2020

banukutlu commented Sep 3, 2020 • edited Loading

ruthtillman commented Sep 4, 2020

cdmo commented Sep 8, 2020

banukutlu commented Sep 8, 2020 • edited Loading

ruthtillman commented Sep 8, 2020

ruthtillman commented Sep 8, 2020

banukutlu commented Sep 8, 2020 • edited Loading

ruthtillman commented Sep 8, 2020

cdmo left a comment • edited Loading

Choose a reason for hiding this comment

ruthtillman commented Sep 9, 2020

cdmo left a comment

Choose a reason for hiding this comment

cdmo left a comment • edited Loading

Choose a reason for hiding this comment

cdmo Sep 11, 2020

Choose a reason for hiding this comment

cdmo left a comment

Choose a reason for hiding this comment

banukutlu Sep 14, 2020

Choose a reason for hiding this comment

cdmo Sep 14, 2020

Choose a reason for hiding this comment

banukutlu commented Sep 14, 2020

cdmo left a comment

Choose a reason for hiding this comment

banukutlu commented Sep 14, 2020

banukutlu commented Aug 31, 2020 •

edited

Loading

banukutlu commented Sep 3, 2020 •

edited

Loading

banukutlu commented Sep 8, 2020 •

edited

Loading

banukutlu commented Sep 8, 2020 •

edited

Loading

cdmo left a comment •

edited

Loading

cdmo left a comment •

edited

Loading