Bug 23218 - Batch patron modification empty attribute causes improper handling of values
Summary: Batch patron modification empty attribute causes improper handling of values
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 19673
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-26 08:03 UTC by David Cook
Modified: 2019-08-13 20:12 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03


Attachments
Bug 23218: Fix shift in patron's attributes in batch patron modification (2.38 KB, patch)
2019-06-29 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23218: Fix shift in patron's attributes in batch patron modification (2.43 KB, patch)
2019-07-05 19:49 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 23218: Fix shift in patron's attributes in batch patron modification (2.49 KB, patch)
2019-07-19 10:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23218: (QA follow-up) Remove dummy value (1.61 KB, patch)
2019-07-19 10:49 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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