Bug 17344 - Can't set guarantor in quick add brief form
Summary: Can't set guarantor in quick add brief form
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 3534
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-25 08:21 UTC by Katrin Fischer
Modified: 2016-11-29 22:22 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17344 - Can't set guarantor in quick add brief form (5.91 KB, patch)
2016-11-21 11:50 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17344 - Can't set guarantor in quick add brief form (6.01 KB, patch)
2016-11-21 12:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17344 - Followup to fix selectors (4.31 KB, patch)
2016-11-22 11:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17344 - Can't set guarantor in quick add brief form (6.01 KB, patch)
2016-11-22 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17344 - Followup to fix selectors (4.37 KB, patch)
2016-11-22 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2016-09-25 08:21:38 UTC
When using the quick add feature it's not possible to add a guarantor.

To test:
- Use quick add form to add a "Kid" (sample patron category)
- "Set patron" and search for a patron to link with
- Click "choose"
- Verify that the guarantor fields in the form remain empty
- Open 'full form' - notice some information is there
Comment 1 Katrin Fischer 2016-11-17 23:28:19 UTC
I think we should try and fix this fast, as quick add is a prominent feature in the new release.
Comment 2 Nick Clemens 2016-11-18 01:41:42 UTC
Working on this, but I have a question:

Guarantor form also adds some address info - Would it be enough to add note to quick add form that this info will be added/saved and to see full form to edit - or should we make these fields (streetnumber, address,address2, city,state,zipcode,country,branchcode) displayed by default on a form with guarantor selector?

(In reply to Katrin Fischer from comment #1)
> I think we should try and fix this fast, as quick add is a prominent feature
> in the new release.
Comment 3 Jonathan Druart 2016-11-18 08:02:10 UTC
What about if we simply remove the support of guarantor for the quick add form?
Comment 4 Katrin Fischer 2016-11-18 08:47:49 UTC
Hm, I wouldn't display by default, a note would work for me.

I am not sure how hard it is to do, but we put some time and effort in to make it work for guarantors, so not sure we should remove it right away.
Comment 5 Nick Clemens 2016-11-21 11:50:44 UTC
Created attachment 57674 [details] [review]
Bug 17344 - Can't set guarantor in quick add brief form

This patch updates the guarantor form to work with the quick add form

To test:
1 - Quick add a 'Kid' patron (or other patron type that may have
        guarantor)
2 - Verify form appears as expected
3 - Test that you can add a patron manually or using selector
4 - Test that deletion works as expected
5 - Test 3 and 4 from the full version of the form
6 - Save or cancel and add a new patron using the full form only
7 - Verify that adding/deleting guarantor works with no errors
8 - Sign off
Comment 6 Owen Leonard 2016-11-21 12:58:05 UTC
Created attachment 57678 [details] [review]
[SIGNED-OFF] Bug 17344 - Can't set guarantor in quick add brief form

This patch updates the guarantor form to work with the quick add form

To test:
1 - Quick add a 'Kid' patron (or other patron type that may have
    guarantor)
2 - Verify form appears as expected
3 - Test that you can add a patron manually or using selector
4 - Test that deletion works as expected
5 - Test 3 and 4 from the full version of the form
6 - Save or cancel and add a new patron using the full form only
7 - Verify that adding/deleting guarantor works with no errors
8 - Sign off

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Jonathan Druart 2016-11-22 07:46:39 UTC
Nick,
At several places you are using a "quick_add_form" selector, this does not look right.
As you are using the id (#) you should not need to specify anything else.
Comment 8 Nick Clemens 2016-11-22 10:36:41 UTC
It is a consequence of cloning the fields, there are doubled ids. I am open to suggestions but this was the easy/fast way to repair this part of the feature.

(In reply to Jonathan Druart from comment #7)
> Nick,
> At several places you are using a "quick_add_form" selector, this does not
> look right.
> As you are using the id (#) you should not need to specify anything else.
Comment 9 Nick Clemens 2016-11-22 11:31:22 UTC
Created attachment 57705 [details] [review]
Bug 17344 - Followup to fix selectors

Add '#' selector to several tags
Fix some spacing
Remove duplicated line

To highlight that all is working set PatronQuickAddFields as below for testing:
streetnumber|address|address2|city|state|zipcode|country|branchcode
Comment 10 Jonathan Druart 2016-11-22 12:54:59 UTC
I don't think we need "#quick_add_form #streetnumber" as a selector, "#streetnumber" should be enough.
Comment 11 Jonathan Druart 2016-11-22 13:19:30 UTC
Created attachment 57708 [details] [review]
Bug 17344 - Can't set guarantor in quick add brief form

This patch updates the guarantor form to work with the quick add form

To test:
1 - Quick add a 'Kid' patron (or other patron type that may have
    guarantor)
2 - Verify form appears as expected
3 - Test that you can add a patron manually or using selector
4 - Test that deletion works as expected
5 - Test 3 and 4 from the full version of the form
6 - Save or cancel and add a new patron using the full form only
7 - Verify that adding/deleting guarantor works with no errors
8 - Sign off

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Jonathan Druart 2016-11-22 13:19:37 UTC
Created attachment 57709 [details] [review]
Bug 17344 - Followup to fix selectors

Add '#' selector to several tags
Fix some spacing
Remove duplicated line

To highlight that all is working set PatronQuickAddFields as below for testing:
streetnumber|address|address2|city|state|zipcode|country|branchcode

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Jonathan Druart 2016-11-22 13:20:51 UTC
Note that some things should be rewritten here.
We should not have 2 elements with the same id at the same time in the DOM, this is semantically wrong.
Comment 14 Katrin Fischer 2016-11-22 13:24:42 UTC
(In reply to Jonathan Druart from comment #13)
> Note that some things should be rewritten here.
> We should not have 2 elements with the same id at the same time in the DOM,
> this is semantically wrong.

It also makes jQuery grumpy - you can't select using the id then in my experience. We have some other places in acq where this happens or happened.
Comment 15 Kyle M Hall 2016-11-29 17:21:54 UTC
Pushed to master for 17.05! Thanks Nick!
Comment 16 Katrin Fischer 2016-11-29 22:22:51 UTC
This patch has been pushed to 16.11.x, will be in 16.11.01!