Bug 24263 - borrowers.relationship should not contain an empty string
Summary: borrowers.relationship should not contain an empty string
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 14570 24215
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-18 13:03 UTC by Jonathan Druart
Modified: 2020-02-05 21:00 UTC (History)
4 users (show)

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


Attachments
Bug 24263: Add tests (1.05 KB, patch)
2019-12-18 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24263: DB changes (973 bytes, patch)
2019-12-18 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24263: Replace borrowers.relationship with NULL when was empty string (1.65 KB, patch)
2019-12-18 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24263: Add tests (1.11 KB, patch)
2019-12-20 17:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24263: DB changes (1.01 KB, patch)
2019-12-20 17:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24263: Replace borrowers.relationship with NULL when was empty string (1.71 KB, patch)
2019-12-20 17:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24263: Add tests (1.17 KB, patch)
2020-01-08 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24263: DB changes (1.07 KB, patch)
2020-01-08 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24263: Replace borrowers.relationship with NULL when was empty string (1.77 KB, patch)
2020-01-08 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24263: (QA follow-up) Atomic update fixes (1.13 KB, patch)
2020-01-08 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-12-18 13:03:37 UTC
It should be set to NULL if not relationship exist.
Otherwise the about page will display a warning about a missing/wrong relationship in data (that is not in the syspref)
Comment 1 Jonathan Druart 2019-12-18 13:12:41 UTC
Created attachment 96418 [details] [review]
Bug 24263: Add tests
Comment 2 Jonathan Druart 2019-12-18 13:12:44 UTC
Created attachment 96419 [details] [review]
Bug 24263: DB changes
Comment 3 Jonathan Druart 2019-12-18 13:12:47 UTC
Created attachment 96420 [details] [review]
Bug 24263: Replace borrowers.relationship with NULL when was empty string

When you create a new patron, its relationship field is populated with
an empty string when we are expecting NULL.

Otherwise the about page will display a warning about a missing/wrong
relationship in data (that is not in the syspref)

Test plan:
0/ Do not apply the patches from this bug report
1/ Create a new patron (child)
2/ Notice that relationship column in DB is set to an empty string
3/ Go to the about page, notice the invalid warning about relationship
values not in the syspref
4/ Apply DB changes and tests
5/ Execute the update DB entry
=> The value in DB is now NULL when was ""
6/ Execute the tests
=> It fails
7/ Apply the last patch
8/ Add and edit a patron (child)
=> relationship is set to NULL in DB
9/ Execute the tests
=> They pass!
Comment 4 Jonathan Druart 2019-12-18 13:13:46 UTC
Upping severity, it should follow the integration of bug 24215 in stable branches.
Comment 5 Martin Renvoize 2019-12-20 17:43:55 UTC
Created attachment 96531 [details] [review]
Bug 24263: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-12-20 17:43:58 UTC
Created attachment 96532 [details] [review]
Bug 24263: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-12-20 17:44:01 UTC
Created attachment 96533 [details] [review]
Bug 24263: Replace borrowers.relationship with NULL when was empty string

When you create a new patron, its relationship field is populated with
an empty string when we are expecting NULL.

Otherwise the about page will display a warning about a missing/wrong
relationship in data (that is not in the syspref)

Test plan:
0/ Do not apply the patches from this bug report
1/ Create a new patron (child)
2/ Notice that relationship column in DB is set to an empty string
3/ Go to the about page, notice the invalid warning about relationship
values not in the syspref
4/ Apply DB changes and tests
5/ Execute the update DB entry
=> The value in DB is now NULL when was ""
6/ Execute the tests
=> It fails
7/ Apply the last patch
8/ Add and edit a patron (child)
=> relationship is set to NULL in DB
9/ Execute the tests
=> They pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-12-20 17:44:32 UTC
Works as expected, Signing off.
Comment 9 Tomás Cohen Arazi 2019-12-30 10:42:42 UTC
Why don't we throw an exception instead?
Comment 10 Jonathan Druart 2020-01-05 21:38:09 UTC
It's following an existing pattern (reset or modify an invalid value in -> store).
Throwing an exception here would mean to catch it in all our Koha::Patron->store. That is much more work, especially to fix a major bug that impacts stable branches.
Comment 11 Tomás Cohen Arazi 2020-01-06 11:11:06 UTC
Fair enough. We will take care of those at some point.
Comment 12 Martin Renvoize 2020-01-07 22:55:34 UTC
Should this be set to PQA Tomas?

The comments read like it should.
Comment 13 Tomás Cohen Arazi 2020-01-08 13:12:37 UTC
Created attachment 96989 [details] [review]
Bug 24263: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2020-01-08 13:12:41 UTC
Created attachment 96990 [details] [review]
Bug 24263: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-01-08 13:12:45 UTC
Created attachment 96991 [details] [review]
Bug 24263: Replace borrowers.relationship with NULL when was empty string

When you create a new patron, its relationship field is populated with
an empty string when we are expecting NULL.

Otherwise the about page will display a warning about a missing/wrong
relationship in data (that is not in the syspref)

Test plan:
0/ Do not apply the patches from this bug report
1/ Create a new patron (child)
2/ Notice that relationship column in DB is set to an empty string
3/ Go to the about page, notice the invalid warning about relationship
values not in the syspref
4/ Apply DB changes and tests
5/ Execute the update DB entry
=> The value in DB is now NULL when was ""
6/ Execute the tests
=> It fails
7/ Apply the last patch
8/ Add and edit a patron (child)
=> relationship is set to NULL in DB
9/ Execute the tests
=> They pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2020-01-08 13:12:48 UTC
Created attachment 96992 [details] [review]
Bug 24263: (QA follow-up) Atomic update fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Martin Renvoize 2020-01-08 14:25:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Joy Nelson 2020-01-18 15:42:55 UTC
Pushed to 19.11.x branch for 19.11.02
Comment 19 Lucas Gass 2020-02-05 21:00:01 UTC
missing dependencies, no backport for 19.05.x