Bug 23218

Summary: Batch patron modification empty attribute causes improper handling of values
Product: Koha Reporter: David Cook <dcook>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, lucas, maryse.simard, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03
Bug Depends on: 19673    
Bug Blocks:    
Attachments: Bug 23218: Fix shift in patron's attributes in batch patron modification
Bug 23218: Fix shift in patron's attributes in batch patron modification
Bug 23218: Fix shift in patron's attributes in batch patron modification
Bug 23218: (QA follow-up) Remove dummy value

Description David Cook 2019-06-26 08:03:28 UTC
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19673 introduced an empty option for Attribute in the Batch Patron Modification.

If you leave "Attribute" empty and click "New", you'll get a new row where you can choose an attribute and specify a value.

When you hit save, the value won't be recorded against the attribute though, because the indexes in the patron_attributes and patron_attributes_value arrays will be out of sync.
Comment 1 Jonathan Druart 2019-06-29 15:24:26 UTC
Created attachment 91130 [details] [review]
Bug 23218: Fix shift in patron's attributes in batch patron modification

When the "new" button is clicked without selecting a patron's attribute, the update will have unexpected behaviors.

There is indeed a difference in the number of patron's attributes and the number of values.

Test plan:
Go to the batch patron modification
Enter a cardnumber
At the bottom of the page, click "new" without selecting an attribute
On the second line select one, and a value (different than an empty
string)
Save
=> The attribute will be updated with an empty string
Comment 2 David Cook 2019-07-01 00:05:52 UTC
I'll be looking at signing off this one when I'm back at the office as well. Thanks, Jonathan!
Comment 3 Maryse Simard 2019-07-05 19:49:17 UTC
Created attachment 91373 [details] [review]
Bug 23218: Fix shift in patron's attributes in batch patron modification

When the "new" button is clicked without selecting a patron's attribute, the update will have unexpected behaviors.

There is indeed a difference in the number of patron's attributes and the number of values.

Test plan:
Go to the batch patron modification
Enter a cardnumber
At the bottom of the page, click "new" without selecting an attribute
On the second line select one, and a value (different than an empty
string)
Save
=> The attribute will be updated with an empty string

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 4 Katrin Fischer 2019-07-06 15:59:57 UTC
value="poeut" ?
Comment 5 David Cook 2019-07-09 04:53:18 UTC
(In reply to Katrin Fischer from comment #4)
> value="poeut" ?

That does look pretty weird...
Comment 6 Nick Clemens 2019-07-19 10:49:48 UTC
Created attachment 91629 [details] [review]
Bug 23218: Fix shift in patron's attributes in batch patron modification

When the "new" button is clicked without selecting a patron's attribute, the update will have unexpected behaviors.

There is indeed a difference in the number of patron's attributes and the number of values.

Test plan:
Go to the batch patron modification
Enter a cardnumber
At the bottom of the page, click "new" without selecting an attribute
On the second line select one, and a value (different than an empty
string)
Save
=> The attribute will be updated with an empty string

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2019-07-19 10:49:51 UTC
Created attachment 91630 [details] [review]
Bug 23218: (QA follow-up) Remove dummy value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2019-07-19 10:50:31 UTC
(In reply to David Cook from comment #5)
> (In reply to Katrin Fischer from comment #4)
> > value="poeut" ?
> 
> That does look pretty weird...

It works with out this, so removed the extra value - it wasn't added if the dropdown was changed so I assume leftover testing code
Comment 9 Martin Renvoize 2019-07-19 12:57:03 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Jonathan Druart 2019-08-05 17:46:33 UTC
(In reply to Nick Clemens from comment #8)
> (In reply to David Cook from comment #5)
> > (In reply to Katrin Fischer from comment #4)
> > > value="poeut" ?
> > 
> > That does look pretty weird...
> 
> It works with out this, so removed the extra value - it wasn't added if the
> dropdown was changed so I assume leftover testing code

Thanks, sorry for that. poeut is weird indeed, it's a typo for pouet ;)
Comment 11 Fridolin Somers 2019-08-06 14:34:07 UTC
Pushed to 19.05.x for 19.05.03
Comment 12 Lucas Gass 2019-08-13 20:12:29 UTC
backported to 18.11.x for 18.11.09