Bug 19452 - The -truncate option in borrowers-force-messaging-defaults.pl should not remove category preferences
Summary: The -truncate option in borrowers-force-messaging-defaults.pl should not remo...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on: 19451
Blocks: 19454
  Show dependency treegraph
 
Reported: 2017-10-12 11:13 UTC by Marcel de Rooy
Modified: 2018-06-04 20:15 UTC (History)
8 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 19452: Replace the truncate option by reset_all (4.48 KB, patch)
2017-10-12 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19452: Remove the truncate option (3.94 KB, patch)
2017-10-16 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19452: Remove the truncate option (4.00 KB, patch)
2017-10-17 12:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19452: Remove the truncate option (4.02 KB, patch)
2017-11-17 20:02 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 19452: Remove the truncate option (4.04 KB, patch)
2018-02-15 13:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19452: Remove the truncate option (4.05 KB, patch)
2018-02-16 15:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19452: [17.11.x] Remove the truncate option (4.02 KB, patch)
2018-02-20 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19452: [17.05.x] Remove the truncate option (3.70 KB, patch)
2018-02-20 14:01 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 11:13:27 UTC

    
Comment 1 Marcel de Rooy 2017-10-12 12:41:00 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-10-12 13:57:03 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-10-16 09:50:04 UTC
Created attachment 68175 [details] [review]
Bug 19452: Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2017-10-17 12:25:09 UTC
Created attachment 68201 [details] [review]
Bug 19452: Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2017-10-17 12:25:47 UTC
Rebased
Comment 6 Charles Farmer 2017-11-03 21:00:23 UTC
Can't really say where the problem lies, but I couldn't apply your patches:
> fatal: sha1 information is lacking or useless (misc/maintenance/borrowers-force-messaging-defaults.pl).
Comment 7 Marcel de Rooy 2017-11-06 07:45:37 UTC
(In reply to Charles Farmer from comment #6)
> Can't really say where the problem lies, but I couldn't apply your patches:
> > fatal: sha1 information is lacking or useless (misc/maintenance/borrowers-force-messaging-defaults.pl).

Thx for trying.
You need the dependency. First git apply 19451.
Comment 8 Charles Farmer 2017-11-17 19:56:23 UTC
You're completely right, I thought I was looking at a "Pushed to master" dependency.

I could easily follow the test plan, and saw no issue after qa1.
Comment 9 Charles Farmer 2017-11-17 20:02:16 UTC
Created attachment 69203 [details] [review]
Bug 19452: Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 10 Jonathan Druart 2018-02-08 18:01:37 UTC
Frédéric, as you are the author of this script can you confirm this change?
Comment 11 Marcel de Rooy 2018-02-15 13:20:24 UTC
Created attachment 71652 [details] [review]
Bug 19452: Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 12 Marcel de Rooy 2018-02-15 13:20:38 UTC
Simple rebase
Comment 13 Katrin Fischer 2018-02-16 05:50:13 UTC
Destroys data unexpectedly... changing this to be a bug. 
Dependency won't get pushed to stable releases as enh tho.
Comment 14 Marcel de Rooy 2018-02-16 06:30:43 UTC
(In reply to Katrin Fischer from comment #13)
> Destroys data unexpectedly... changing this to be a bug. 
> Dependency won't get pushed to stable releases as enh tho.

Are you working on it now? This comment might suggest so..
Comment 15 Katrin Fischer 2018-02-16 06:31:38 UTC
Not working on it and not saying so - more indicating that a patch for stable vresions might be nice :)
Comment 16 Nick Clemens 2018-02-16 15:10:22 UTC
Created attachment 71815 [details] [review]
Bug 19452: Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Jonathan Druart 2018-02-18 19:49:41 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 18 Marcel de Rooy 2018-02-19 09:00:08 UTC
(In reply to Katrin Fischer from comment #15)
> more indicating that a patch for
> stable vresions might be nice :)

Will submit one
Comment 19 Marcel de Rooy 2018-02-20 14:00:50 UTC
Created attachment 72000 [details] [review]
Bug 19452: [17.11.x] Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Marcel de Rooy 2018-02-20 14:01:17 UTC
Created attachment 72001 [details] [review]
Bug 19452: [17.05.x] Remove the truncate option

The truncate option is not really useful. Its result is probably not what
most users of this script expect or need.
It truncates both tables borrower_message_preferences and
borrower_message_transport_preferences. This (unfortunately) includes
deleting messaging preferences for patron categories. After that, adding
preferences again will not add categories again, but only borrower
preferences which are all disabled.

Furthermore, we do not need to disable the foreign key check. Neither
do we actually need to truncate, deleting records seems sufficient.
Also deleting transport preferences is not needed, since it will be
done by a cascade from messaging preferences. Note that the subsequent
call of SetMessagingPreferencesFromDefaults will already delete the
records.

This makes it possible to remove the truncate option altogether.

Test plan:
[1] Select a patron category (say ST) and change days_in_advance to x.
[2] Select a ST patron and set days_advance to y in his msg prefs.
[3] Run borrowers-force-messaging-defaults.pl -doit
[4] Verify that the patron has been reset to the default prefs (incl.
    value x in days_in_advance).
[5] Verify that the patron category prefs are still intact.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Fridolin SOMERS 2018-03-21 08:06:13 UTC
Can we push to stable branches even if they do not contain Bug 19451 ?
Comment 22 Marcel de Rooy 2018-03-21 12:00:59 UTC
(In reply to Fridolin SOMERS from comment #21)
> Can we push to stable branches even if they do not contain Bug 19451 ?

Probably. Careful rebase?
Comment 23 Nick Clemens 2018-03-23 12:48:31 UTC
Awsome work all! Pushed to stable for 17.11.04
Comment 24 Fridolin SOMERS 2018-03-27 08:50:13 UTC
Pushed to 17.05.x for v17.05.10

Realy strange that this script does not have the .pl extension :/