Bug 21346 - Clean up dialogs in returns.pl
Summary: Clean up dialogs in returns.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: Macintosh All
: P5 - low major
Assignee: Christopher Brannon
QA Contact: Josef Moravec
URL:
Keywords:
: 13153 17339 18490 19720 (view as bug list)
Depends on:
Blocks: 18490 21944 22896 23007 23145 24257 24434
  Show dependency treegraph
 
Reported: 2018-09-14 19:31 UTC by Christopher Brannon
Modified: 2020-11-30 21:45 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05, 18.05.12
Circulation function:


Attachments
[PATCH] Bug 21346: Remove old hold/transfer dialog (5.60 KB, patch)
2018-09-14 19:47 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Remove old hold/transfer dialog (5.65 KB, patch)
2018-09-14 20:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21346: Update/Fix dialogs - Fix hold waiting (14.30 KB, patch)
2018-09-15 22:18 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Update buttons on transfer only (1.65 KB, patch)
2018-09-15 23:18 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Update buttons on transfer only (1.71 KB, patch)
2018-09-15 23:58 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Update/Fix dialogs - Fix hold waiting (14.36 KB, patch)
2018-09-16 17:11 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Update buttons on transfer only (1.76 KB, patch)
2018-09-16 17:11 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Update/Fix dialogs - Fix hold waiting (7.28 KB, patch)
2018-10-01 01:33 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Update/Fix dialogs - Fix hold waiting (7.28 KB, patch)
2018-10-01 01:49 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Fixes dual modals on waiting hold. (941 bytes, patch)
2018-10-18 21:45 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Restore values needed for buttons (1.82 KB, patch)
2018-10-24 22:44 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Streamline logic in holds and transfers (2.44 KB, patch)
2018-11-01 20:26 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Remove double dialog (1.22 KB, patch)
2018-11-01 21:12 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Convert dialogs to modals (15.93 KB, patch)
2018-11-02 20:10 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Convert dialogs to modals. (15.66 KB, patch)
2018-11-04 03:54 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Reroute waiting hold from wrong branch (2.01 KB, patch)
2018-11-08 00:00 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Streamline logic in holds and transfers (2.49 KB, patch)
2018-11-09 17:30 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Remove double dialog (1.27 KB, patch)
2018-11-09 17:30 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Convert dialogs to modals. (15.70 KB, patch)
2018-11-09 17:30 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Reroute waiting hold from wrong branch (2.06 KB, patch)
2018-11-09 17:30 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21346: Remove inline onclicks per comment 42 (9.48 KB, patch)
2018-12-05 20:37 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21346: Streamline logic in holds and transfers (2.53 KB, patch)
2019-04-09 16:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21346: Remove double dialog (1.31 KB, patch)
2019-04-09 16:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21346: Convert dialogs to modals. (15.75 KB, patch)
2019-04-09 16:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21346: Remove inline onclicks per comment 42 (9.52 KB, patch)
2019-04-09 16:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21346: Streamline logic in holds and transfers (2.60 KB, patch)
2019-04-12 12:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 21346: Remove double dialog (1.38 KB, patch)
2019-04-12 12:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 21346: Convert dialogs to modals. (15.81 KB, patch)
2019-04-12 12:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 21346: Remove inline onclicks per comment 42 (9.59 KB, patch)
2019-04-12 12:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2018-09-14 19:31:06 UTC
Someone added a new modal for holds/transfers in return.tt (yay!).  They left the old dialog in the background, and if you ignore the modal, it remains.  To replicate:

1) Place a hold on an item from Branch A to be delivered to Branch B.
2) Checkin in the item at Branch A and confirm the hold to transfer.
3) Be human and make a mistake.  Login as Branch C and check the item in.
4) Note the great modal to redirect the hold.  Note that the old dialog is behind it!
Comment 1 Christopher Brannon 2018-09-14 19:47:45 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2018-09-14 20:31:37 UTC Comment hidden (obsolete)
Comment 3 Christopher Brannon 2018-09-14 21:45:09 UTC
I found that this broke something else.  Back to the drawing board!
Comment 4 Christopher Brannon 2018-09-15 21:53:47 UTC
I managed to fix the patch AND fix the bug with the waiting holds being checked in at the wrong location that do nothing!  Test plan to come.
Comment 5 Christopher Brannon 2018-09-15 22:18:32 UTC Comment hidden (obsolete)
Comment 6 Spencer 2018-09-15 22:58:35 UTC
Everything is functional- however when an item is checked in to transfer back to its home branch (not on hold) there is no confirm option- only a force to print a slip.  WE don't print slips for non-hold transfers.

If that scenario offered a simple "confirm" choice, it would work well.
Comment 7 Christopher Brannon 2018-09-15 23:18:30 UTC Comment hidden (obsolete)
Comment 8 Christopher Brannon 2018-09-15 23:20:39 UTC
Added button per Spencer's suggestion.  Thanks for testing!  Please try again.
Comment 9 ByWater Sandboxes 2018-09-15 23:58:59 UTC Comment hidden (obsolete)
Comment 10 Christopher Brannon 2018-09-16 06:00:14 UTC
*** Bug 17339 has been marked as a duplicate of this bug. ***
Comment 11 Christopher Brannon 2018-09-16 16:12:37 UTC
(In reply to ByWater Sandboxes from comment #9)
> Created attachment 78916 [details] [review] [review]
> Bug 21346: Update buttons on transfer only
> 
> Added a confirm only button and updated text on print button for the
> transfer dialog so you are not forced to print.
> 
> Signed-off-by: Spencer <ssmith3@mckinneytexas.org>

Spencer, when signing off, if there is more than one patch involved, you need to sign off on the whole thing, not just a single patch.  Have Ed show you how.  :)
Comment 12 ByWater Sandboxes 2018-09-16 17:11:30 UTC Comment hidden (obsolete)
Comment 13 ByWater Sandboxes 2018-09-16 17:11:33 UTC Comment hidden (obsolete)
Comment 14 Christopher Brannon 2018-09-26 20:06:32 UTC
*** Bug 13153 has been marked as a duplicate of this bug. ***
Comment 15 Christopher Brannon 2018-09-26 20:09:05 UTC
*** Bug 19720 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Druart 2018-09-30 22:55:01 UTC
This is bugfix, not an enhancement, right?
I would suggest to provide a patch which would not contain the indentation changes, that will generate conflicts and make the backport harder.
Comment 17 Christopher Brannon 2018-10-01 00:06:22 UTC
(In reply to Jonathan Druart from comment #16)
> This is bugfix, not an enhancement, right?
> I would suggest to provide a patch which would not contain the indentation
> changes, that will generate conflicts and make the backport harder.

It is both.  Please explain.  I don't understand the issue.
Comment 18 Christopher Brannon 2018-10-01 01:00:54 UTC
(In reply to Christopher Brannon from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > This is bugfix, not an enhancement, right?
> > I would suggest to provide a patch which would not contain the indentation
> > changes, that will generate conflicts and make the backport harder.
> 
> It is both.  Please explain.  I don't understand the issue.

Never mind.  I think I understand.
Comment 19 Christopher Brannon 2018-10-01 01:33:30 UTC Comment hidden (obsolete)
Comment 20 Christopher Brannon 2018-10-01 01:39:59 UTC
Resubmitted patch, removing indent changes per request and combined patches.  No new code.
Comment 21 Christopher Brannon 2018-10-01 01:49:03 UTC Comment hidden (obsolete)
Comment 22 Christopher Brannon 2018-10-01 01:50:19 UTC
Fixed typo in plan.  We should be good now.
Comment 23 Christopher Brannon 2018-10-01 02:07:56 UTC
Found a logic problem.  Hold-found2 modal triggering on top of hold-found1 modal if you check an item that is already waiting at that location.  I will go in and update the logic to not show hold-found2 modal if waiting.  I will put this back to needs sign-off when complete.
Comment 24 Christopher Brannon 2018-10-18 21:45:55 UTC Comment hidden (obsolete)
Comment 25 Christopher Brannon 2018-10-18 21:47:14 UTC
I've isolated the flaw.  We are good to go again.  Test plan remains the same.  I will also submit a patch to clean up the tabs.
Comment 26 Christopher Brannon 2018-10-18 21:48:55 UTC
Changing this to a bug fix, as it fixes dialog issues and a known bug.  Not really adding anything new in terms of features.
Comment 27 Christopher Brannon 2018-10-19 21:03:25 UTC
Found another issue.  While I have fixed the routing issue, an item marked waiting and turned in to the wrong branch will re-route the item correctly, but does not set the item back to top priority.  Looking into this.  Failing for now.
Comment 28 Christopher Brannon 2018-10-24 22:44:23 UTC Comment hidden (obsolete)
Comment 29 Christopher Brannon 2018-10-31 14:57:35 UTC
I am going to revamp this bug, once again.  There are some other issues at hand that need to be done to handle this more appropriately:

* I will be mapping out hold and transfer behaviors
* Clean up some logic - some of it is a bit cobbled together:  For example, found is used specificly when a hold is found, but has been used for some simple transfers as well.
* Clarify some variables - found is too generic
* Add a routine for correcting waiting holds - there is no current method for reverting a hold from waiting to in transit in returns.
* Finish converting dialogs to modals.
Comment 30 Christopher Brannon 2018-11-01 20:26:10 UTC Comment hidden (obsolete)
Comment 31 Christopher Brannon 2018-11-01 21:12:12 UTC Comment hidden (obsolete)
Comment 32 Christopher Brannon 2018-11-02 20:10:55 UTC Comment hidden (obsolete)
Comment 33 Christopher Brannon 2018-11-02 20:15:43 UTC
Okay, patches for cleanup are set.  Now addressing the waiting hold at the wrong location.

It must:
* Bring up a modal.
* Only prompt to send the item to the pickup location.
* Update current location in item.
* Initiate a transfer.
* Update the reserve from a W to T in the found field.
* Remove the waiting date in the reserves.
Comment 34 Christopher Brannon 2018-11-04 03:54:57 UTC Comment hidden (obsolete)
Comment 35 Christopher Brannon 2018-11-08 00:00:18 UTC Comment hidden (obsolete)
Comment 36 Christopher Brannon 2018-11-08 00:02:16 UTC
Finally.  I think this was the best approach.  Sorry for the number of changes.  If someone thinks I need to create a test for the modification to C4/Circulation.pm, I can try to do that to.
Comment 37 Christopher Brannon 2018-11-08 18:55:18 UTC
*** Bug 18490 has been marked as a duplicate of this bug. ***
Comment 38 ByWater Sandboxes 2018-11-09 17:30:03 UTC
Created attachment 82189 [details] [review]
Bug 21346: Streamline logic in holds and transfers

1) Replaced comparison with string to make testing branch easier.
2) Replaced useless 0 values with undef.

Does not change any behavior, only simplifies code.

Signed-off-by: Lisette <lisetteslatah@gmail.com>
Comment 39 ByWater Sandboxes 2018-11-09 17:30:07 UTC
Created attachment 82190 [details] [review]
Bug 21346: Remove double dialog

To Test:
1) Place a hold on an item for pickup at another branch.
2) Check in item to trigger hold and transfer.
3) Check in item again without changing location.  Note modal AND old
dialog behind modal.
4) Apply patch.
5) Check in item again without changing location.  Note modal without
the old dialog.

Signed-off-by: Lisette <lisetteslatah@gmail.com>
Comment 40 ByWater Sandboxes 2018-11-09 17:30:12 UTC
Created attachment 82191 [details] [review]
Bug 21346: Convert dialogs to modals.

This addresses most of the transfer dialogs.  There are some dialogs
that I have not converted because I don't know what triggers them,
therefore I cannot test them.

The following scenarios have been addresses, and should be tested:

FOR TRANSFERS

1) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Don't)
     * Should give 3 options - Yes, Yes with print, or No.
     * Yes and Yes with print should trigger a transfer back home.
     * No should do nothing.
     * Print should open a window for printing, with correct
     information.
     * All three options should close the modal.

     TO TEST:
     1) Set AutomaticItemREturn to Don't.
     2) Check in an item with no issues, holds or transfers set, at a
     location other than the owning library.
     3) Test conditions above.

2) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Do)
    * Should give 2 options - Print or OK.
    * Should automatically set transfer.
    * Print should open a window for printing, with correct information.
    * Both buttons should close modal.

    TO TEST:
    1) Set AutomaticItemReturn to Do.
    2) Check in an item with no issues, holds or transfers set, at a
    location other than the owning library.
    3) Test conditions above.

3) Checkin with no issues or holds, but transfer already set
    * Should give 3 options - OK, Print or Cancel.
    * OK and print should not touch existing transfer.
    * Cancel should remove the exisiting transfer.
    * Print should open a window for printing, with correct information.
    * All three options should close the modal.

    TO TEST:
    1) Check in an item following step 2 of either test above.
    2) Check in item again, while a transfer exists.
    3) Test conditions above.

WRONG BRANCH

4) If AllowReturnToBranch is not set "to any library", and the item is not checked in at the appropriate branch, the wrong-branch-modal pops up:
    * Should give 1 option - OK.
    * Should not check anything in or initiate a transfer.
    * OK should close the modal.

    TO TEST:
    1) Set AllowReturnToBranch to "only the library the item is from".
    You can test the other settings, as long as you pay attention to
    where you are checking the item in at.
    2) Check in an item at a branch other than the owning library.
    3) Test conditions above.

Signed-off-by: Lisette <lisetteslatah@gmail.com>
Comment 41 ByWater Sandboxes 2018-11-09 17:30:16 UTC
Created attachment 82192 [details] [review]
Bug 21346: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.

Signed-off-by: Lisette <lisetteslatah@gmail.com>
Comment 42 Josef Moravec 2018-12-03 20:54:04 UTC
Overall it looks good, just one thing - please don't use inline javascript (onclick="...")

And could you please separate the fix for rerouting wating hold to its own bug report?
Comment 43 Josef Moravec 2018-12-03 20:54:47 UTC
(In reply to Josef Moravec from comment #42)
> 
> And could you please separate the fix for rerouting wating hold to its own
> bug report?

And it would be nice to have regression test here...
Comment 44 Christopher Brannon 2018-12-03 21:29:38 UTC
(In reply to Josef Moravec from comment #43)
> (In reply to Josef Moravec from comment #42)
> > 
> > And could you please separate the fix for rerouting wating hold to its own
> > bug report?
> 
> And it would be nice to have regression test here...

I don't know what that is.
Comment 45 Christopher Brannon 2018-12-04 16:56:35 UTC
Ignore.  Everyone calls it something different.  Okay. The waiting patch is on bug 21944.  I will add the test there.  Since nothing else involves a pm here, I am setting this back to signed off.
Comment 46 Josef Moravec 2018-12-05 06:32:05 UTC
(In reply to Josef Moravec from comment #42)
> Overall it looks good, just one thing - please don't use inline javascript
> (onclick="...")


Please, fix this.
Comment 47 Josef Moravec 2018-12-05 06:34:37 UTC
(In reply to Christopher Brannon from comment #45)
> Ignore.  Everyone calls it something different.  Okay. The waiting patch is
> on bug 21944.  I will add the test there.  Since nothing else involves a pm
> here, I am setting this back to signed off.

I'm not native speaker, so maybe I'm sometimes not using the right words, sorry ;)

Good job
Comment 48 Christopher Brannon 2018-12-05 20:37:32 UTC
Created attachment 82891 [details] [review]
Bug 21346: Remove inline onclicks per comment 42

To test:
1)  Apply patch.
2)  Check that there is no changed behavior with buttons or checkboxes.
Comment 49 Christopher Brannon 2018-12-05 20:38:51 UTC
(In reply to Josef Moravec from comment #46)
> (In reply to Josef Moravec from comment #42)
> > Overall it looks good, just one thing - please don't use inline javascript
> > (onclick="...")
> 
> 
> Please, fix this.

Done.  Waiting for sign off.
Comment 50 Christopher Brannon 2018-12-05 20:39:53 UTC
Lisette, when you get a chance, would you test again.  Make sure there are no differences with buttons or checkboxes.  Thanks!

Christopher
Comment 51 Mark Tompsett 2019-01-09 15:32:38 UTC
Comment on attachment 82891 [details] [review]
Bug 21346: Remove inline onclicks per comment 42

Review of attachment 82891 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
@@ +672,2 @@
>                      [% ELSE %]
> +                        <input type="checkbox" id="return_date_override_remember" name="return_date_override_remember" />

While the code below actually fixes some of the inline javascript that you corrected, I don't see anything that is the equivalent of forcing the focus to barcode for this. I'll look at the full file to confirm if there is something already.
Comment 52 Liz Rea 2019-03-05 20:43:34 UTC
Mark,

Does this qualify as a FQA for you?

Cheers,
Liz
Comment 53 Liz Rea 2019-04-09 16:22:08 UTC
Created attachment 87625 [details] [review]
Bug 21346: Streamline logic in holds and transfers

1) Replaced comparison with string to make testing branch easier.
2) Replaced useless 0 values with undef.

Does not change any behavior, only simplifies code.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 54 Liz Rea 2019-04-09 16:22:12 UTC
Created attachment 87626 [details] [review]
Bug 21346: Remove double dialog

To Test:
1) Place a hold on an item for pickup at another branch.
2) Check in item to trigger hold and transfer.
3) Check in item again without changing location.  Note modal AND old
dialog behind modal.
4) Apply patch.
5) Check in item again without changing location.  Note modal without
the old dialog.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 55 Liz Rea 2019-04-09 16:22:16 UTC
Created attachment 87627 [details] [review]
Bug 21346: Convert dialogs to modals.

This addresses most of the transfer dialogs.  There are some dialogs
that I have not converted because I don't know what triggers them,
therefore I cannot test them.

The following scenarios have been addresses, and should be tested:

FOR TRANSFERS

1) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Don't)
     * Should give 3 options - Yes, Yes with print, or No.
     * Yes and Yes with print should trigger a transfer back home.
     * No should do nothing.
     * Print should open a window for printing, with correct
     information.
     * All three options should close the modal.

     TO TEST:
     1) Set AutomaticItemREturn to Don't.
     2) Check in an item with no issues, holds or transfers set, at a
     location other than the owning library.
     3) Test conditions above.

2) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Do)
    * Should give 2 options - Print or OK.
    * Should automatically set transfer.
    * Print should open a window for printing, with correct information.
    * Both buttons should close modal.

    TO TEST:
    1) Set AutomaticItemReturn to Do.
    2) Check in an item with no issues, holds or transfers set, at a
    location other than the owning library.
    3) Test conditions above.

3) Checkin with no issues or holds, but transfer already set
    * Should give 3 options - OK, Print or Cancel.
    * OK and print should not touch existing transfer.
    * Cancel should remove the exisiting transfer.
    * Print should open a window for printing, with correct information.
    * All three options should close the modal.

    TO TEST:
    1) Check in an item following step 2 of either test above.
    2) Check in item again, while a transfer exists.
    3) Test conditions above.

WRONG BRANCH

4) If AllowReturnToBranch is not set "to any library", and the item is not checked in at the appropriate branch, the wrong-branch-modal pops up:
    * Should give 1 option - OK.
    * Should not check anything in or initiate a transfer.
    * OK should close the modal.

    TO TEST:
    1) Set AllowReturnToBranch to "only the library the item is from".
    You can test the other settings, as long as you pay attention to
    where you are checking the item in at.
    2) Check in an item at a branch other than the owning library.
    3) Test conditions above.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 56 Liz Rea 2019-04-09 16:22:19 UTC
Created attachment 87628 [details] [review]
Bug 21346: Remove inline onclicks per comment 42

To test:
1)  Apply patch.
2)  Check that there is no changed behavior with buttons or checkboxes.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 57 Kyle M Hall (khall) 2019-04-12 12:35:16 UTC
Created attachment 87894 [details] [review]
Bug 21346: Streamline logic in holds and transfers

1) Replaced comparison with string to make testing branch easier.
2) Replaced useless 0 values with undef.

Does not change any behavior, only simplifies code.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 58 Kyle M Hall (khall) 2019-04-12 12:35:27 UTC
Created attachment 87895 [details] [review]
Bug 21346: Remove double dialog

To Test:
1) Place a hold on an item for pickup at another branch.
2) Check in item to trigger hold and transfer.
3) Check in item again without changing location.  Note modal AND old
dialog behind modal.
4) Apply patch.
5) Check in item again without changing location.  Note modal without
the old dialog.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 59 Kyle M Hall (khall) 2019-04-12 12:35:31 UTC
Created attachment 87896 [details] [review]
Bug 21346: Convert dialogs to modals.

This addresses most of the transfer dialogs.  There are some dialogs
that I have not converted because I don't know what triggers them,
therefore I cannot test them.

The following scenarios have been addresses, and should be tested:

FOR TRANSFERS

1) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Don't)
     * Should give 3 options - Yes, Yes with print, or No.
     * Yes and Yes with print should trigger a transfer back home.
     * No should do nothing.
     * Print should open a window for printing, with correct
     information.
     * All three options should close the modal.

     TO TEST:
     1) Set AutomaticItemREturn to Don't.
     2) Check in an item with no issues, holds or transfers set, at a
     location other than the owning library.
     3) Test conditions above.

2) Checkin with no issue, hold or transfer; not at home (AutomaticItemReturn set to Do)
    * Should give 2 options - Print or OK.
    * Should automatically set transfer.
    * Print should open a window for printing, with correct information.
    * Both buttons should close modal.

    TO TEST:
    1) Set AutomaticItemReturn to Do.
    2) Check in an item with no issues, holds or transfers set, at a
    location other than the owning library.
    3) Test conditions above.

3) Checkin with no issues or holds, but transfer already set
    * Should give 3 options - OK, Print or Cancel.
    * OK and print should not touch existing transfer.
    * Cancel should remove the exisiting transfer.
    * Print should open a window for printing, with correct information.
    * All three options should close the modal.

    TO TEST:
    1) Check in an item following step 2 of either test above.
    2) Check in item again, while a transfer exists.
    3) Test conditions above.

WRONG BRANCH

4) If AllowReturnToBranch is not set "to any library", and the item is not checked in at the appropriate branch, the wrong-branch-modal pops up:
    * Should give 1 option - OK.
    * Should not check anything in or initiate a transfer.
    * OK should close the modal.

    TO TEST:
    1) Set AllowReturnToBranch to "only the library the item is from".
    You can test the other settings, as long as you pay attention to
    where you are checking the item in at.
    2) Check in an item at a branch other than the owning library.
    3) Test conditions above.

Signed-off-by: Lisette <lisetteslatah@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Kyle M Hall (khall) 2019-04-12 12:35:34 UTC
Created attachment 87898 [details] [review]
Bug 21346: Remove inline onclicks per comment 42

To test:
1)  Apply patch.
2)  Check that there is no changed behavior with buttons or checkboxes.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Nick Clemens (kidclamp) 2019-04-16 13:23:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 62 Martin Renvoize (ashimema) 2019-04-25 13:22:49 UTC
Pushed to 18.11.x series for 18.11.05
Comment 63 Lucas Gass (lukeg) 2019-04-26 01:52:04 UTC
backported to 18.05.x for 18.05.12
Comment 64 Fridolin Somers 2019-04-30 06:02:30 UTC
Does not apply on 17.11.x.
But since its nearly end of life we may let it like that.
Comment 65 Magnus Enger 2019-05-26 15:45:41 UTC
I have a library that objects rather strongly to some of the changes introduced here. The main problem is the message that says an item needs a transfer. They used to be able to keep on returning books even if that message appeared. Now their workflow is disrupted and they have to click the OK button to make the modal go away. 

Could we make it optional if the popup message is a modal or not? (On a separate bug, of course. Just commenting here to get the opinion of the people on this bug.)
Comment 66 Magnus Enger 2019-05-26 15:46:32 UTC
PS. The library has AutomaticItemReturn = Do.
Comment 67 Owen Leonard 2019-05-28 15:51:14 UTC
(In reply to Magnus Enger from comment #65)

> Could we make it optional if the popup message is a modal or not?

Agree 100%
Comment 68 George Williams (NEKLS) 2019-06-27 04:34:43 UTC
(In reply to Magnus Enger from comment #65)

> 
> Could we make it optional if the popup message is a modal or not?

I also agree - whether or not the message is a modal or not should be tied to a system preference so that libraries that want the modal to interrupt staff workflow (like me) can have that and those that do not want the workflow to be interrupted can have that also.

George
Comment 69 Martin Renvoize (ashimema) 2019-06-27 06:02:09 UTC
Sounds like a consensus to me.. could we open a new bug for this please :)