Bug 19451 - Let borrowers-force-messaging-defaults.pl optionally add preferences only when not already present
Summary: Let borrowers-force-messaging-defaults.pl optionally add preferences only whe...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on: 19472
Blocks: 19452
  Show dependency treegraph
 
Reported: 2017-10-12 10:15 UTC by Marcel de Rooy
Modified: 2018-04-06 13:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This report adds the command-line option 'no-overwrite' so that you can add preferences only when they are not yet present (in other words: skip patrons that already set their prefs).


Attachments
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl (3.79 KB, patch)
2017-10-12 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl (3.83 KB, patch)
2017-10-17 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl (3.83 KB, patch)
2017-10-30 12:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl (3.89 KB, patch)
2018-02-07 21:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19451: (QA follow-up) Change parameter name no_overwrite to no-overwrite for consistency (1.62 KB, patch)
2018-02-07 21:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19451: (QA follow-up) Replace weird subquery (1.60 KB, patch)
2018-02-09 11:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-10-12 10:15:54 UTC
A welcome addition to borrowers-force-messaging-defaults.pl would be to allow adding the messaging defaults only to patrons without such preferences.
This will be made possible with the empty_only option. It copies the prefs only when the borrower prefs are empty (read: are not present).
Comment 1 Marcel de Rooy 2017-10-12 10:40:16 UTC
Renaming empty_only to no_overwrite which might be better understood.
Comment 2 Marcel de Rooy 2017-10-12 10:49:55 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-10-17 12:21:08 UTC
Created attachment 68200 [details] [review]
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl

This option allows you to add preferences only when they are not yet
present. In other words: skip patrons that already set their prefs.

Test plan:
[1] Delete all borrower messaging prefs for a patron.
[2] Run borrowers-force-messaging-defaults.pl -no_overwrite -doit
    Verify that the patron now has default msg preferences.
[3] Change his settings and make them non-default.
    For instance, increase days in advance.
[4] Run borrowers-force-messaging-defaults.pl -no_overwrite -doit
    Verify that the patron still has the non-default settings.
[5] Run borrowers-force-messaging-defaults.pl -doit
    Verify that the patron msg prefs have been overwritten.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2017-10-17 12:21:37 UTC
Rebased
Comment 5 Owen Leonard 2017-10-30 12:55:57 UTC
Created attachment 68844 [details] [review]
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl

This option allows you to add preferences only when they are not yet
present. In other words: skip patrons that already set their prefs.

Test plan:
[1] Delete all borrower messaging prefs for a patron.
[2] Run borrowers-force-messaging-defaults.pl -no_overwrite -doit
    Verify that the patron now has default msg preferences.
[3] Change his settings and make them non-default.
    For instance, increase days in advance.
[4] Run borrowers-force-messaging-defaults.pl -no_overwrite -doit
    Verify that the patron still has the non-default settings.
[5] Run borrowers-force-messaging-defaults.pl -doit
    Verify that the patron msg prefs have been overwritten.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Josef Moravec 2018-02-07 21:14:06 UTC
Created attachment 71327 [details] [review]
Bug 19451: Add no_overwrite option to borrowers-force-messaging-defaults.pl

This option allows you to add preferences only when they are not yet
present. In other words: skip patrons that already set their prefs.

Test plan:
[1] Delete all borrower messaging prefs for a patron.
[2] Run borrowers-force-messaging-defaults.pl -no-overwrite -doit
    Verify that the patron now has default msg preferences.
[3] Change his settings and make them non-default.
    For instance, increase days in advance.
[4] Run borrowers-force-messaging-defaults.pl -no-overwrite -doit
    Verify that the patron still has the non-default settings.
[5] Run borrowers-force-messaging-defaults.pl -doit
    Verify that the patron msg prefs have been overwritten.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2018-02-07 21:14:12 UTC
Created attachment 71328 [details] [review]
Bug 19451: (QA follow-up) Change parameter name no_overwrite to no-overwrite for consistency

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Jonathan Druart 2018-02-08 18:04:30 UTC
+    if( $no_overwrite ) {
+        $sql .= " AND (SELECT COUNT(*) FROM borrower_message_preferences mp WHERE mp.borrowernumber=bo.borrowernumber) = 0"
+    }

This is weird, I think you want to add a join here instead.
Comment 9 Marcel de Rooy 2018-02-09 11:04:55 UTC
Created attachment 71367 [details] [review]
Bug 19451: (QA follow-up) Replace weird subquery

As requested, we add a JOIN and make the SELECT distinct.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested that no-overwrite still works as expected.
Comment 10 Jonathan Druart 2018-02-12 21:08:46 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 11 Nick Clemens 2018-02-16 01:51:19 UTC
Awesome work all! Enhancement, not backported to stable.