webclient: Marking item damaged does not trigger prompt for billing details

[Kathy Lussier]

In the XUL client, if the 'Charge item price when marked damaged' Library Setting is enabled an item is marked damaged from Item Status or the Check In screen, a prompt appears asking the user to confirm that the item status should change to damaged. Another prompt then appears requesting asking if the user wants to charge the full item change, wants to change the price, or doesn't want to charge the patron at all.

Neither prompt appears when marking an item damaged in the web client.

[Kathy Lussier (klussier)]

Also, the status of the copy does not change to Damaged.

[Kathy Lussier (klussier)]

Also adding a note that we wouldn't object if the web client implementation put both alerts into one dialog to save the time of the user. It would need to be configured so that the window only displays the status change alert for libraries that don't have the 'Charge item price when marked damage' enabled and display the rest of the options when the setting is enabled.

[Kathy Lussier (klussier)]

Correction to comment #1.

From the item status screen, the status of the copy is successfully changed to Damaged and I see no errors in the Console.

From the checkin and renew items screens, I get the following error in the Console when I try to Mark the Item Damaged.

mark damaged failed: Event: :DAMAGE_CHARGE ->
(anonymous) @ circ.js:1123
(anonymous) @ angular.js:16606
$eval @ angular.js:17913
$digest @ angular.js:17727
(anonymous) @ angular.js:17952
e @ angular.js:6035
(anonymous) @ angular.js:6314

In this case, the status does not change to Damaged.

If the Library Setting to charge a fee for damaged items is not enabled, the Mark Damaged action in check in and renew items works as expected.

[Andrea Neiman (aneiman)]

Confirmed in 2.12, except I'm getting the same console error "mark damaged failed: Event: :DAMAGE_CHARGE" whether I'm in Item Status, Checkin, or Renew, and in all locations the item doesn't get marked damaged, but stays in its prior status.

If the Library Setting for charging a fee is set to False, items are correctly changed to the "damaged" status.

[Kyle Huckins (khuckins)]

I've taken a quick look at this, so here's a quick technical breakdown of what I've found so far, in the interest of having something to reference. The full response when I have the library setting set to an amount contains the following:
1. an empty description(odd)
2. an empty ilsevent(odd)
3. a payload consisting of the charge and the circ
4. the pid
5. the servertime
6. the stacktrace
8. the textcode

The first two things seem odd to me, but everything else comes back with what I'd expect it to.

The stacktrace leads to these files:
/usr/local/share/perl/5.22.1/OpenILS/Application/Circ.pm:1428
/usr/local/share/perl/5.22.1/OpenILS/Application/Circ.pm:1291
/usr/local/share/perl/5.22.1/OpenSRF/Application.pm:628

Application.pm:628 is just pointing to the code that returns a response through OpenSRF, so I'm not looking too heavily there.

Circ.pm:1291 leads to the subroutine above what we'll look at in the next one, handle_mark_damaged, mark_item. Specifically it's pointing the API call over to handle_mark_damaged.

Circ.pm:1428 leads to the end of handle_mark_damaged, checking if $apply DOESN'T exist. $apply is set early on to either the argument apply_fines or a blank string. So my assumption here is that it's not getting that argument as it expects to.

If I add an object with apply_fines: true to service.mark_damaged() (staff/circ/services/circ.js) during its API call, the item gets marked damaged and the patron is also billed the correct amount. So it looks like all this needs is an argument checking for the library setting.

[Mike Rylander (mrylander)]

Thanks, Kyle.

It looks like the intended workflow is for the client to receive the object you're getting now, use that to pop up a modal that offers to charge the amount described by {payload=>{charge=>$total}}, or not, and then submit the call again with a value for "apply_fines" of either "noapply" (don't charge the total amount) or a truthy value (do apply them).

What I'm unsure of based on inspecting only handle_mark_damaged is whether there is an YAOUS that allows bypassing the interstitial prompt and simply submitting a truthy "apply_fines" argument.

Does that all make sense?

[Terran McCanna (tmccanna)]

Confirmed that this is still a problem in 2.12.4. We're not getting the prompt at all in the web client. In the xul client we're getting the first and second prompt, but getting a FixMe error after choosing an option on the second prompt. (Our test server is down at the moment, so I'm not sure exactly what the FixMe message is.)

[Terran McCanna (tmccanna)]

Pls ignore my comment about xul - turns out we had a problem with our test server and now it's working as expected in the xul client.

Confirmed that it isn't working at all in 2.12.4.

[Kyle Huckins (khuckins)]

Alright, I've pushed up(and then quickly rebased after noticing it needed an update to the latest version of master) a branch that solves this: http://git.evergreen-ils.org?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1659181-item-damaged-prompt

It makes some changes to how egCirc.mark_damaged is called - namely we're calling it for every item in order to keep things working similarly to the XUL client's current functionality. Additionally, we're passing through the copy id, barcode, and if applicable, the circulation library(to determine what/if we'll have a fee for the item).

[Terran McCanna (tmccanna)]

Will test

[Kyle Huckins (khuckins)]

As a heads up, we just noted an issue that managed to slip past after our rebase - there's no longer an add_barcode_to_list function in the holds service. Currently it will error without updating the list on the holds screen, but we have a fix for that that can get pushed up within the week.

[Kyle Huckins (khuckins)]

Branch has been updated to resolve this issue

[Kathy Lussier (klussier)]

Hi Kyle,
I loaded the patch from the updated branch this morning, but I'm having trouble with it. When I try marking an item damaged on the Check In Screen, the action is failing with no feedback for the user. This occurs when the 'Charge item price when marked damaged' setting is enabled and when it is left unset. I see the following in my Console:

angular.min.js:119 Error: [$injector:unpr] http://errors.angularjs.org/1.5.11/$injector/unpr?p0=egBillingProvider%20%3C-%20egBilling
    at angular.min.js:6
    at angular.min.js:43
    at Object.d [as get] (angular.min.js:41)
    at angular.min.js:43
    at d (angular.min.js:41)
    at e (angular.min.js:41)
    at Object.invoke (angular.min.js:41)
    at R.instance (angular.min.js:91)
    at ui-bootstrap-tpls.min.js:2
    at angular.min.js:132

On the item status screen, the action works as expected when the Library Setting is unset. When the setting is enabled, I get a prompt telling me the item will be marked damage. When I click 'Submit' to confirm, the action fails without feedback to the user. I see the following in my Console:

mark damaged failed: Event: :DAMAGE_CHARGE ->

[Kyle Huckins (khuckins)]

Looking into this, I hadn't encountered this but now I am - interestingly the list view seems to work just fine, but the detail view gives that problem. I'll report my findings and bring in a repatch.

[Kyle Huckins (khuckins)]

It seems I somehow managed to completely miss the checkin interface. I've updated to include that particular interface, as well as resolve the issues with the item details screen, and this should now cover the following interfaces:
Item Status(List)
Item Status(Detail)
Patron Holds
Checkin

[Kathy Lussier (klussier)]

Thank you for the update Kyle. I'm still having trouble with the branch and have not yet seen the new prompt.

1. When the library setting is enabled (I usually enabled it for the consortium, but also tested it when it was set for BR1), here is what I'm seeing:

- On the checkin screen, regardless of the status the copy was put in after checkin, marking the item fails without feedback to the user. I'm still getting mark damaged failed: Event: :DAMAGE_CHARGE -> in the Console.

- in item status list and detail view, if the item has an in transit status, holds shelf, or reshelving status, I get the same behavior that I see on the checkin screen.

- in item status list and detail view, if the item has an available status, the item is successfully marked as damaged, but I don't get the prompt asking for billing details. I suspect that, in this case, because I'm working with the Concerto data, these items don't have a previous patron to bill whereas the other ones tested from item status did have a previous circ. I haven't yet tested an item with an available status that has a previous circ.

2. I found a few more interfaces that need to be touched. I was surprised when you mentioned the patron holds interface because I never realized this action was available here. However, after looking further, I see the action is also available from the Holds Pull List, Holds Shelf, and record Holds interfaces.

3. If the setting is left Unset, items are marked damaged as expected. However, I have a couple of quibbles here. These quibbles appear to be pre-existing conditions in the web client and could be submitted as a new bug if they aren't addressed in this bug.

- In the xul client, after marking an item damaged, there is a dialog that confirms the item has been marked damaged. There is nothing that provides this feedback to the user in the web client. Preferably, we could use a toast notification confirming that it has been marked damaged so that the user doesn't need to click a button to proceed.

- In the old client, when we mark an item damaged on the item status list screen, we get a new row added to the item status window that shows the changed status. In the web client, we get the new row, but it still shows the old status even if the damaged status was successfully applied.

- Similarly, in the old client, when we mark an item damaged on the item status detail screen, the screen refreshes after the new status is applied and shows that the status has been changed to damaged. This doesn't happen in the web client. I do see that the web client's Mark Missing action on the item status detail view behaves this way.

[Kyle Huckins (khuckins)]

I've been testing with just the same - CONS and BR1, separately.

I'm not receiving this error on available or checked out statuses in the checkin interface after my last patch, so this is pretty bizarre. I'll test things out with other statuses, as you're noting that that error is occurring with those statuses. The only thing I could think of that would cause issues with those is that I'm passing in the copy's circ lib, and with those statuses, it wouldn't necessarily have one.

You're right with not getting the prompt - it's because without a previous circ, there's nobody to bill.

I'll take a look at those interfaces.

For the quibbles:
1. The toast notification seems like a good plan.
2. The item status list should be updating the status on the new row already - I'm not sure why this isn't the case on your end - I'll see if I can replicate this.
3. This should also currently be happening - not a total refresh, but a refresh of the information under the tabs, which would include showing the status changing to damaged.

[Kyle Huckins (khuckins)]

As a quick update, I've made a push with the additions to those interfaces, but have not been able to replicate the bugs noted or quibbles 2 & 3. Next up is getting those toasts to work.

[Kyle Huckins (khuckins)]

I've made a few commits since my last message, and have just squashed and pushed them up - the code itself should be good to go as is now. Some modifications have been made - the cause of the error you were running into was due to some of the introduced code trying(and mostly succeeding) to handle something already handled in the API. Problem being it was accounting for one of the charge settings, but not both of them. That code has been reworked to allow the API to do its thing unhindered. Additionally, the mark damaged modal has been simplified, and no longer calls up the patron billing modal to change a value(yay less modals!).

Branch continues to be found here: http://git.evergreen-ils.org?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1659181-item-damaged-prompt

A couple things I've noticed:

1. The Patron Billing interface doesn't seem to display bill notes properly - in some cases it does, but on full details and the regular patron bills interface, there doesn't seem to be any sign of manually entered notes, despite the notes displaying in the DB.

2. A suggestion from Bill Erickson was that there are a number of places in cat/item/app.js where "egItem" is imported, but referred to as itemSvc, which was pretty confusing to go through for the both of us.

[Kathy Lussier (klussier)]

Thanks Kyle!

As mentioned in IRC, the branch needs some cleaning up. When I cherry-picked http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=0910392cdaf5fb7eaf1c8cf4c74c33533fd0e288 , I still encounterd several of the same problems described above. When I merged the branch, I had to resolve some conflicts, but I was finally able to get the Damaged Items dialog to appear (hooray!).

However, the commits will need to be picked before merging, so I want to make sure I'm picking everything that's required. I thought that there may be a commit I was missing further down in the branch, but I didn't see anything when I performed git cherry.

Now that I have the popup, I have two issues to report:

1. When marking the item damaged while it is still checked out to the patron, the item remains on the patron's Items Out list. This differs from the xul client's behavior where the item was checked in as part of the marking damaged process. This occurs regardless of the state of the 'Charge item price when marked damage' setting.

2. I love that we eliminated one modal here. However, I find the 'charge fees' and 'no charge' buttons confusing. Using the 'no charge' button before clicking submit works as expected, and no charge is assessed to the user. However, I think staff may be confused because the price of the item still remains in the charge box. Maybe it would be better if clicking 'no charge' added visual reinforcement by changing the price to 0. Clicking 'Charge Fees', then, could reinstate the price of the item in that box.

IMO, #1 should be addressed before merging the code and #2 could be addressed in a new bug once we have the basic functionality in place.

Many thanks for all of your work on this Kyle!

[Kyle Huckins (khuckins)]

For issue 1 - It looks like the XUL code handles the checkin in this process, but it seems like something that could be more efficiently handled in the API, so my plan is to add this in to the API for marking an item as damaged, and doing so in backward-compatible way, flagging it so it knows if it needs to start checking the item in or not(effectively, making sure it knows that the request is coming from either the web staff client or the xul client).

[Kyle-huckins (kyle-huckins)]

I've pushed up the changes here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1659181-item-damaged-prompt-fix

This should resolve both of the issues brought up - Issue 2's needed changes happened naturally after the modifications needed to properly handle the API change.

[Kathy Lussier (klussier)]

Thank you Kyle! It works for me.

I still see some of the feedback issues I mentioned in comment #16, but I'll file new bugs to addressed those. I signed off on your commit and merged it to master and 3.0. There was a conflict that I wasn't comfortable resolving for 2.12, so I didn't go any further than 3.0