Bug 18595 - Move C4::Members::Messaging to Koha namespace
Summary: Move C4::Members::Messaging to Koha namespace
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords:
Depends on: 17499
Blocks: 14806 25228
  Show dependency treegraph
 
Reported: 2017-05-12 13:07 UTC by Lari Taskula
Modified: 2020-11-18 04:30 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.02 KB, patch)
2017-05-12 13:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.37 KB, patch)
2017-05-12 13:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (6.88 KB, patch)
2017-05-12 13:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (6.57 KB, patch)
2017-05-12 13:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (19.81 KB, patch)
2017-05-12 13:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (5.63 KB, patch)
2017-05-12 13:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (7.37 KB, patch)
2017-05-12 13:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.02 KB, patch)
2017-05-29 11:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.37 KB, patch)
2017-05-29 11:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.72 KB, patch)
2017-05-29 11:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.27 KB, patch)
2017-05-29 11:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (20.46 KB, patch)
2017-05-29 11:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.12 KB, patch)
2017-05-29 11:17 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (7.76 KB, patch)
2017-05-29 11:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Fix failing test in Circulation.t (936 bytes, patch)
2017-05-29 14:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.16 KB, patch)
2017-05-29 14:51 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.50 KB, patch)
2017-05-29 14:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.85 KB, patch)
2017-05-29 14:54 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.44 KB, patch)
2017-05-29 14:55 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (20.59 KB, patch)
2017-05-29 14:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.39 KB, patch)
2017-05-29 15:00 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (7.89 KB, patch)
2017-05-29 15:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: (follow-up) Fix failing test in Circulation.t (1.05 KB, patch)
2017-05-29 15:03 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.45 KB, patch)
2017-05-29 15:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (7.87 KB, patch)
2017-05-29 15:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Fix failing test in Circulation.t (1.05 KB, patch)
2017-05-29 15:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.52 KB, patch)
2017-05-29 15:28 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.52 KB, patch)
2017-05-29 15:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (7.87 KB, patch)
2017-05-29 15:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: (follow-up) Fix failing test in Circulation.t (1.05 KB, patch)
2017-05-29 15:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.16 KB, patch)
2017-12-01 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.49 KB, patch)
2017-12-01 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.87 KB, patch)
2017-12-01 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.49 KB, patch)
2017-12-01 16:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.87 KB, patch)
2017-12-01 16:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.16 KB, patch)
2017-12-01 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.49 KB, patch)
2017-12-01 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.87 KB, patch)
2017-12-01 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.34 KB, patch)
2017-12-01 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (20.40 KB, patch)
2017-12-01 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.49 KB, patch)
2017-12-01 16:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.58 KB, patch)
2017-12-01 16:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.27 KB, patch)
2017-12-01 16:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.22 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.55 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.93 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.40 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (20.46 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.55 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.64 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.33 KB, patch)
2017-12-08 21:27 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.21 KB, patch)
2019-10-08 14:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.65 KB, patch)
2019-10-08 14:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.67 KB, patch)
2019-10-08 14:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.34 KB, patch)
2019-10-08 14:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (19.92 KB, patch)
2019-10-08 14:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.55 KB, patch)
2019-10-08 14:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.65 KB, patch)
2019-10-08 14:31 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (7.19 KB, patch)
2019-10-08 14:32 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Validate days_in_advance (1.24 KB, patch)
2019-10-09 09:44 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.09 KB, patch)
2019-10-25 13:07 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Validate days_in_advance (1.24 KB, patch)
2019-10-25 13:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.21 KB, patch)
2020-04-22 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.67 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.67 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.30 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (19.93 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.55 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.60 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.09 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Validate days_in_advance (1.24 KB, patch)
2020-04-22 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.21 KB, patch)
2020-09-16 06:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.68 KB, patch)
2020-09-16 06:55 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.67 KB, patch)
2020-09-16 06:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.32 KB, patch)
2020-09-16 06:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (19.88 KB, patch)
2020-09-16 06:57 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.56 KB, patch)
2020-09-16 06:59 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.61 KB, patch)
2020-09-16 07:00 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.25 KB, patch)
2020-09-16 07:01 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Validate days_in_advance (1.25 KB, patch)
2020-09-16 07:02 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging (3.27 KB, patch)
2020-10-22 08:11 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions (7.74 KB, patch)
2020-10-22 08:12 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults (7.73 KB, patch)
2020-10-22 08:13 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Unit tests preparing for GetMessagingPreferences move (5.38 KB, patch)
2020-10-22 08:14 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences (19.93 KB, patch)
2020-10-22 08:16 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference (6.61 KB, patch)
2020-10-22 08:17 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Remove references to C4::Members::Messaging (8.66 KB, patch)
2020-10-22 08:17 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Disable digest checkbox when forced on (6.30 KB, patch)
2020-10-22 08:18 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: Validate days_in_advance (1.30 KB, patch)
2020-10-22 08:19 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 18595: (follow-up) Disable transports if missing contact information (11.70 KB, patch)
2020-10-30 11:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Catch message preference exceptions in OPAC (8.09 KB, patch)
2020-11-05 17:33 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Disable transports if missing contact information (11.83 KB, patch)
2020-11-05 20:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Disable transports if missing contact information (13.00 KB, patch)
2020-11-06 09:56 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Catch message preference exceptions in OPAC (8.08 KB, patch)
2020-11-06 09:57 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18595: (follow-up) Remove duplicated logic (1.25 KB, patch)
2020-11-18 04:30 UTC, Lari Taskula
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2017-05-12 13:07:02 UTC
Bug 17499 adds Koha-objects for messaging preferences.
Comment 1 Lari Taskula 2017-05-12 13:29:16 UTC
Created attachment 63435 [details] [review]
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging

This patch adds unit tests for Koha::Patron::Categories->default_messaging. Tests
will help preparing for moving C4::Members::Messaging to Koha-objects.

To test:
1. Run t/db_dependent/Koha/Patron/Categories.t
Comment 2 Lari Taskula 2017-05-12 13:29:26 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2017-05-12 13:29:36 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2017-05-12 13:29:46 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2017-05-12 13:29:57 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2017-05-12 13:30:08 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2017-05-12 13:30:20 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2017-05-12 13:39:11 UTC
I attached the patches that I was working on last year.

I would like to point out that the work was left unfinished. Especially the patches without a proper test plan are probably somewhat untested and need more testing. There could be new occurrences to C4::Members::Messaging that also need to be handled by these patches. Please keep this in mind if testing. I remember there was some bug related to category messaging preference view and I'm not sure if that got fixed.

I was hoping someone in the community finds this work useful and wants to take it to the finish line. Feel free to steal this Bug or help fixing issues by follow-ups.
Comment 9 Lari Taskula 2017-05-29 11:15:35 UTC Comment hidden (obsolete)
Comment 10 Lari Taskula 2017-05-29 11:15:47 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2017-05-29 11:15:58 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2017-05-29 11:16:08 UTC Comment hidden (obsolete)
Comment 13 Lari Taskula 2017-05-29 11:16:19 UTC Comment hidden (obsolete)
Comment 14 Lari Taskula 2017-05-29 11:17:51 UTC Comment hidden (obsolete)
Comment 15 Lari Taskula 2017-05-29 11:18:02 UTC Comment hidden (obsolete)
Comment 16 Lari Taskula 2017-05-29 11:21:26 UTC
(In reply to Lari Taskula from comment #8)
> I remember there was some bug related to category messaging
> preference view and I'm not sure if that got fixed.
Couldn't reproduce issues regarding patron category messaging preferences anymore. Switched status to Needs Signoff, but please see my thoughts in comment #8
Comment 17 Marc Véron 2017-05-29 13:52:54 UTC
(In reply to Lari Taskula from comment #4)
> Created attachment 63438 [details] [review] [review]
> Bug 18595: Unit tests preparing for GetMessagingPreferences move
> 
> This patch prepares removing GetMessagingPreferences by adding some useful
> unit
> tests for subroutines that the move targets.
> 
> Reserves.t already covers print notice, but this patch adds tests for a set
> transport type.
> 
> To test:
> 1. Run t/db_dependent/Circulation.t
> 2. Run t/db_dependent/Reserves.t
> 3. Observe test pass

prove -v t/db_dependent/Circulation.t does not pass 
I get:
# Subtest: SendCirculationAlert test
    1..4
    # No tests run!
not ok 96 - No tests run for subtest "SendCirculationAlert test"

t/db_dependent/Reserves.t returns green
Comment 18 Lari Taskula 2017-05-29 14:08:32 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2017-05-29 14:15:46 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2017-05-29 14:36:26 UTC
Went through all patches / test plans, no issues detected after the followup-fix
Will sign off
Comment 21 Marc Véron 2017-05-29 14:51:54 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2017-05-29 14:53:12 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2017-05-29 14:54:21 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2017-05-29 14:55:40 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2017-05-29 14:57:12 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2017-05-29 15:00:47 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2017-05-29 15:02:46 UTC Comment hidden (obsolete)
Comment 28 Marc Véron 2017-05-29 15:03:45 UTC Comment hidden (obsolete)
Comment 29 Lari Taskula 2017-05-29 15:20:53 UTC Comment hidden (obsolete)
Comment 30 Lari Taskula 2017-05-29 15:21:08 UTC Comment hidden (obsolete)
Comment 31 Lari Taskula 2017-05-29 15:21:19 UTC Comment hidden (obsolete)
Comment 32 Marc Véron 2017-05-29 15:28:53 UTC Comment hidden (obsolete)
Comment 33 Marc Véron 2017-05-29 15:40:01 UTC Comment hidden (obsolete)
Comment 34 Marc Véron 2017-05-29 15:41:19 UTC Comment hidden (obsolete)
Comment 35 Marc Véron 2017-05-29 15:42:14 UTC Comment hidden (obsolete)
Comment 36 Lari Taskula 2017-12-01 16:19:09 UTC Comment hidden (obsolete)
Comment 37 Lari Taskula 2017-12-01 16:19:23 UTC Comment hidden (obsolete)
Comment 38 Lari Taskula 2017-12-01 16:19:35 UTC Comment hidden (obsolete)
Comment 39 Lari Taskula 2017-12-01 16:20:16 UTC Comment hidden (obsolete)
Comment 40 Lari Taskula 2017-12-01 16:20:33 UTC Comment hidden (obsolete)
Comment 41 Lari Taskula 2017-12-01 16:21:03 UTC Comment hidden (obsolete)
Comment 42 Lari Taskula 2017-12-01 16:21:14 UTC Comment hidden (obsolete)
Comment 43 Lari Taskula 2017-12-01 16:21:26 UTC Comment hidden (obsolete)
Comment 44 Lari Taskula 2017-12-01 16:21:40 UTC Comment hidden (obsolete)
Comment 45 Lari Taskula 2017-12-01 16:21:54 UTC Comment hidden (obsolete)
Comment 46 Lari Taskula 2017-12-01 16:22:06 UTC Comment hidden (obsolete)
Comment 47 Lari Taskula 2017-12-01 16:22:27 UTC Comment hidden (obsolete)
Comment 48 Lari Taskula 2017-12-01 16:22:42 UTC Comment hidden (obsolete)
Comment 49 Lari Taskula 2017-12-01 16:24:18 UTC
Rebased on updated Bug 17499 and added one more patch.
Comment 50 David Bourgault 2017-12-08 21:27:20 UTC
Created attachment 69667 [details] [review]
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging

This patch adds unit tests for Koha::Patron::Categories->default_messaging. Tests
will help preparing for moving C4::Members::Messaging to Koha-objects.

To test:
1. Run t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 51 David Bourgault 2017-12-08 21:27:23 UTC
Created attachment 69668 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions

This patch moves C4::Members::GetMessagingOptions to
Koha::Patron::Message::Preferences->get_options.

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

This patch also removes previously unused GetMessagingOptions in opac-messaging.pl

To test:

Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingOptions"
   (grep -r 'GetMessagingOptions').
3. Observe no results in source code.

---- OPAC ----
1. Login to OPAC.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.

---- INTRANET ----
1. Login to intranet.
2. Go to edit patron's details and scroll down to their messaging preferences.
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.
6. Go to Administration -> Patron categories -> Edit (for any category)
7. Scroll down to observe default messaging preferences for this patron category
8. Observe the preferences looking normal and as they should be.
9. Set any combination of messaging preferences.
10. Observe them apply as expected, repeat step 9-10 as much as you like.
11. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
12. Fill in required data, do not modify messaging preferences, click Save.
13. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 52 David Bourgault 2017-12-08 21:27:26 UTC
Created attachment 69669 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults

This patch moves C4::Members::SetMessagingPreferencesFromDefaults to
Koha::Patron->set_default_messaging_preferences

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreferencesFromDefaults"
       (grep -r 'SetMessagingPreferencesFromDefaults').
3. Observe no results in source code.

---- INTRANET ----
1. Login to intranet.
2. Go to Administration -> Patron categories -> Edit (for any category)
3. Scroll down to observe default messaging preferences for this patron category
4. Observe the preferences, remember them for the next step.
5. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
6. Look at messaging preferences. They should be same as in step 4.
7. Fill in required data, do not modify messaging preferences, click Save.
8. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 53 David Bourgault 2017-12-08 21:27:29 UTC
Created attachment 69670 [details] [review]
Bug 18595: Unit tests preparing for GetMessagingPreferences move

This patch prepares removing GetMessagingPreferences by adding some useful unit
tests for subroutines that the move targets.

Reserves.t already covers print notice, but this patch adds tests for a set
transport type.

To test:
1. Run t/db_dependent/Circulation.t
2. Run t/db_dependent/Reserves.t
3. Observe test pass

Works together with followup patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 54 David Bourgault 2017-12-08 21:27:33 UTC
Created attachment 69671 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences

This patch moves C4::Members::Messaging::GetMessagingPreferences to
Koha::Patron::Message::Preference(s)
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingPreferences"
       (grep -r 'GetMessagingPreferences').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 55 David Bourgault 2017-12-08 21:27:36 UTC
Created attachment 69672 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference

This patch moves C4::Members::Messaging::SetMessagingPreference to
Koha::Patron::Message::Preference
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreference"
       (grep -r 'SetMessagingPreference').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Works as expected. QA tools fails with:
FAIL	C4/Members/Messaging.pm pod empty section in previous paragraph
Can be fixed in a followup.
Signed-off-by: Marc Véron <veron@veron.ch>

QA tools pass now OK
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 56 David Bourgault 2017-12-08 21:27:40 UTC
Created attachment 69673 [details] [review]
Bug 18595: Remove references to C4::Members::Messaging

To test:
1. grep -r 'C4::Members::Messaging'
   This command should not produce any results in Koha source code after this
   patch.
2. Check the pages touched by this commit
3. Try setting and viewing patron and patron category messaging preferences

As this completes the move from C4::Members::Messaging to new Koha-objects for
messaging preferences, everything should be ready for proper testing.

Make sure you have EnhancedMessagingPreferences system preference enabled.
Then, try creating new categories and setting default messaging preferences,
as well as editing the messaging preferences of the already existing categories.
Try adding new patrons and make sure the default patron messaging preferences
are by default the same that you set for that category. Edit already exisiting
patrons' messaging preferences and make sure changes are applied. Observe
no crash at any point, otherwise fail this patch. Don't forget to test both
intranet and OPAC.

Finally, run unit tests:
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 57 David Bourgault 2017-12-08 21:27:43 UTC
Created attachment 69674 [details] [review]
Bug 18595: Disable digest checkbox when forced on

To test:
1. Force a digest on via message_transports table
   (delete rows where message_attribute is 1 and is_digest is 0)
2. Go to either OPAC or staff client messaging preferences
3. Observe digest checkbox is disabled for message attribute 1
   (normally Item due row)

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 58 Alex Arnaud 2018-02-08 09:30:32 UTC
Starting QA now...
Comment 59 Alex Arnaud 2018-02-08 10:11:45 UTC Comment hidden (obsolete)
Comment 60 Alex Arnaud 2018-02-26 14:58:47 UTC
Conflicts in tools/import_borrowers.pl
Comment 61 Lari Taskula 2019-10-08 14:30:30 UTC Comment hidden (obsolete)
Comment 62 Lari Taskula 2019-10-08 14:30:50 UTC Comment hidden (obsolete)
Comment 63 Lari Taskula 2019-10-08 14:31:03 UTC Comment hidden (obsolete)
Comment 64 Lari Taskula 2019-10-08 14:31:15 UTC Comment hidden (obsolete)
Comment 65 Lari Taskula 2019-10-08 14:31:28 UTC Comment hidden (obsolete)
Comment 66 Lari Taskula 2019-10-08 14:31:40 UTC Comment hidden (obsolete)
Comment 67 Lari Taskula 2019-10-08 14:31:52 UTC Comment hidden (obsolete)
Comment 68 Lari Taskula 2019-10-08 14:32:11 UTC Comment hidden (obsolete)
Comment 69 Lari Taskula 2019-10-08 14:33:45 UTC Comment hidden (obsolete)
Comment 70 Lari Taskula 2019-10-09 09:44:40 UTC Comment hidden (obsolete)
Comment 71 Lari Taskula 2019-10-25 13:07:59 UTC Comment hidden (obsolete)
Comment 72 Lari Taskula 2019-10-25 13:08:14 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2020-04-21 12:51:14 UTC
I've tried to rebase the patch but ended up with 

error: sha1 information is lacking or useless (C4/Auth_with_ldap.pm).
Comment 74 Jonathan Druart 2020-04-22 09:25:57 UTC
Created attachment 103444 [details] [review]
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging

This patch adds unit tests for Koha::Patron::Categories->default_messaging. Tests
will help preparing for moving C4::Members::Messaging to Koha-objects.

To test:
1. Run t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 75 Jonathan Druart 2020-04-22 09:26:01 UTC
Created attachment 103445 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions

This patch moves C4::Members::GetMessagingOptions to
Koha::Patron::Message::Preferences->get_options.

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

This patch also removes previously unused GetMessagingOptions in opac-messaging.pl

To test:

Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingOptions"
   (grep -r 'GetMessagingOptions').
3. Observe no results in source code.

---- OPAC ----
1. Login to OPAC.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.

---- INTRANET ----
1. Login to intranet.
2. Go to edit patron's details and scroll down to their messaging preferences.
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.
6. Go to Administration -> Patron categories -> Edit (for any category)
7. Scroll down to observe default messaging preferences for this patron category
8. Observe the preferences looking normal and as they should be.
9. Set any combination of messaging preferences.
10. Observe them apply as expected, repeat step 9-10 as much as you like.
11. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
12. Fill in required data, do not modify messaging preferences, click Save.
13. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 76 Jonathan Druart 2020-04-22 09:26:05 UTC
Created attachment 103446 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults

This patch moves C4::Members::SetMessagingPreferencesFromDefaults to
Koha::Patron->set_default_messaging_preferences

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreferencesFromDefaults"
       (grep -r 'SetMessagingPreferencesFromDefaults').
3. Observe no results in source code.

---- INTRANET ----
1. Login to intranet.
2. Go to Administration -> Patron categories -> Edit (for any category)
3. Scroll down to observe default messaging preferences for this patron category
4. Observe the preferences, remember them for the next step.
5. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
6. Look at messaging preferences. They should be same as in step 4.
7. Fill in required data, do not modify messaging preferences, click Save.
8. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 77 Jonathan Druart 2020-04-22 09:26:09 UTC
Created attachment 103447 [details] [review]
Bug 18595: Unit tests preparing for GetMessagingPreferences move

This patch prepares removing GetMessagingPreferences by adding some useful unit
tests for subroutines that the move targets.

Reserves.t already covers print notice, but this patch adds tests for a set
transport type.

To test:
1. Run t/db_dependent/Circulation.t
2. Run t/db_dependent/Reserves.t
3. Observe test pass

Works together with followup patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 78 Jonathan Druart 2020-04-22 09:26:13 UTC
Created attachment 103448 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences

This patch moves C4::Members::Messaging::GetMessagingPreferences to
Koha::Patron::Message::Preference(s)
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingPreferences"
       (grep -r 'GetMessagingPreferences').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 79 Jonathan Druart 2020-04-22 09:26:18 UTC
Created attachment 103449 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference

This patch moves C4::Members::Messaging::SetMessagingPreference to
Koha::Patron::Message::Preference
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreference"
       (grep -r 'SetMessagingPreference').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Works as expected. QA tools fails with:
FAIL	C4/Members/Messaging.pm pod empty section in previous paragraph
Can be fixed in a followup.
Signed-off-by: Marc Véron <veron@veron.ch>

QA tools pass now OK
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 80 Jonathan Druart 2020-04-22 09:26:22 UTC
Created attachment 103450 [details] [review]
Bug 18595: Remove references to C4::Members::Messaging

To test:
1. grep -r 'C4::Members::Messaging'
   This command should not produce any results in Koha source code after this
   patch.
2. Check the pages touched by this commit
3. Try setting and viewing patron and patron category messaging preferences

As this completes the move from C4::Members::Messaging to new Koha-objects for
messaging preferences, everything should be ready for proper testing.

Make sure you have EnhancedMessagingPreferences system preference enabled.
Then, try creating new categories and setting default messaging preferences,
as well as editing the messaging preferences of the already existing categories.
Try adding new patrons and make sure the default patron messaging preferences
are by default the same that you set for that category. Edit already exisiting
patrons' messaging preferences and make sure changes are applied. Observe
no crash at any point, otherwise fail this patch. Don't forget to test both
intranet and OPAC.

Finally, run unit tests:
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 81 Jonathan Druart 2020-04-22 09:26:26 UTC
Created attachment 103451 [details] [review]
Bug 18595: Disable digest checkbox when forced on

To test:
1. Force a digest on via message_transports table
   (delete rows where message_attribute is 1 and is_digest is 0)
SQL: delete from message_transports where message_attribute_id=1 and is_digest=0;
2. Go to either OPAC or staff client messaging preferences
3. Observe digest checkbox is disabled for message attribute 1
   (normally Item due row)

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 82 Jonathan Druart 2020-04-22 09:26:29 UTC
Created attachment 103452 [details] [review]
Bug 18595: Validate days_in_advance
Comment 83 Katrin Fischer 2020-07-04 21:18:00 UTC
The dependent patch needs a little bit of touching up - moving this to BLOCKED until we have taken care of it.
Comment 84 Katrin Fischer 2020-07-30 22:29:20 UTC
These have a ton of conflicts with the rebased patches for bug 17499 I was just looking it. I assume it would make sense to test them together - could these be rebased?
Comment 85 Katrin Fischer 2020-08-05 10:40:55 UTC
(In reply to Katrin Fischer from comment #84)
> These have a ton of conflicts with the rebased patches for bug 17499 I was
> just looking it. I assume it would make sense to test them together - could
> these be rebased?

The dependent bug is PQA now and Joubu has looked over it... would be a great moment to get the ball rolling here again as well :)
Comment 86 Lari Taskula 2020-09-07 10:18:23 UTC
Nice to see these moving on. Unfortunately I am currently unavailable to contribute further and have removed myself as assignee. If someone wants to finish the work here, it would be greatly appreciated.
Comment 87 Emmi Takkinen 2020-09-16 06:54:06 UTC
Created attachment 110151 [details] [review]
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging

This patch adds unit tests for Koha::Patron::Categories->default_messaging. Tests
will help preparing for moving C4::Members::Messaging to Koha-objects.

To test:
1. Run t/db_dependent/Koha/Patron/Categories.t
Comment 88 Emmi Takkinen 2020-09-16 06:55:06 UTC
Created attachment 110152 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions

This patch moves C4::Members::GetMessagingOptions to
Koha::Patron::Message::Preferences->get_options.

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

This patch also removes previously unused GetMessagingOptions in opac-messaging.pl

To test:

Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingOptions"
   (grep -r 'GetMessagingOptions').
3. Observe no results in source code.

---- OPAC ----
1. Login to OPAC.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.

---- INTRANET ----
1. Login to intranet.
2. Go to edit patron's details and scroll down to their messaging preferences.
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.
6. Go to Administration -> Patron categories -> Edit (for any category)
7. Scroll down to observe default messaging preferences for this patron category
8. Observe the preferences looking normal and as they should be.
9. Set any combination of messaging preferences.
10. Observe them apply as expected, repeat step 9-10 as much as you like.
11. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
12. Fill in required data, do not modify messaging preferences, click Save.
13. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 89 Emmi Takkinen 2020-09-16 06:56:19 UTC
Created attachment 110153 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults

This patch moves C4::Members::SetMessagingPreferencesFromDefaults to
Koha::Patron->set_default_messaging_preferences

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreferencesFromDefaults"
       (grep -r 'SetMessagingPreferencesFromDefaults').
3. Observe no results in source code.

---- INTRANET ----
1. Login to intranet.
2. Go to Administration -> Patron categories -> Edit (for any category)
3. Scroll down to observe default messaging preferences for this patron category
4. Observe the preferences, remember them for the next step.
5. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
6. Look at messaging preferences. They should be same as in step 4.
7. Fill in required data, do not modify messaging preferences, click Save.
8. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 90 Emmi Takkinen 2020-09-16 06:57:02 UTC
Created attachment 110154 [details] [review]
Bug 18595: Unit tests preparing for GetMessagingPreferences move

This patch prepares removing GetMessagingPreferences by adding some useful unit
tests for subroutines that the move targets.

Reserves.t already covers print notice, but this patch adds tests for a set
transport type.

To test:
1. Run t/db_dependent/Circulation.t
2. Run t/db_dependent/Reserves.t
3. Observe test pass

Works together with followup patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 91 Emmi Takkinen 2020-09-16 06:57:55 UTC
Created attachment 110155 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences

This patch moves C4::Members::Messaging::GetMessagingPreferences to
Koha::Patron::Message::Preference(s)
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingPreferences"
       (grep -r 'GetMessagingPreferences').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 92 Emmi Takkinen 2020-09-16 06:59:17 UTC
Created attachment 110156 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference

This patch moves C4::Members::Messaging::SetMessagingPreference to
Koha::Patron::Message::Preference
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreference"
       (grep -r 'SetMessagingPreference').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Works as expected. QA tools fails with:
FAIL	C4/Members/Messaging.pm pod empty section in previous paragraph
Can be fixed in a followup.
Signed-off-by: Marc Véron <veron@veron.ch>

QA tools pass now OK
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 93 Emmi Takkinen 2020-09-16 07:00:13 UTC
Created attachment 110157 [details] [review]
Bug 18595: Remove references to C4::Members::Messaging

To test:
1. grep -r 'C4::Members::Messaging'
   This command should not produce any results in Koha source code after this
   patch.
2. Check the pages touched by this commit
3. Try setting and viewing patron and patron category messaging preferences

As this completes the move from C4::Members::Messaging to new Koha-objects for
messaging preferences, everything should be ready for proper testing.

Make sure you have EnhancedMessagingPreferences system preference enabled.
Then, try creating new categories and setting default messaging preferences,
as well as editing the messaging preferences of the already existing categories.
Try adding new patrons and make sure the default patron messaging preferences
are by default the same that you set for that category. Edit already exisiting
patrons' messaging preferences and make sure changes are applied. Observe
no crash at any point, otherwise fail this patch. Don't forget to test both
intranet and OPAC.

Finally, run unit tests:
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 94 Emmi Takkinen 2020-09-16 07:01:20 UTC
Created attachment 110158 [details] [review]
Bug 18595: Disable digest checkbox when forced on

To test:
1. Force a digest on via message_transports table
   (delete rows where message_attribute is 1 and is_digest is 0)
SQL: delete from message_transports where message_attribute_id=1 and is_digest=0;
2. Go to either OPAC or staff client messaging preferences
3. Observe digest checkbox is disabled for message attribute 1
   (normally Item due row)

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 95 Emmi Takkinen 2020-09-16 07:02:13 UTC
Created attachment 110159 [details] [review]
Bug 18595: Validate days_in_advance
Comment 96 Emmi Takkinen 2020-09-16 07:03:30 UTC
Rebased and tested against current master and bug 17499.
Comment 97 Katrin Fischer 2020-09-16 07:38:24 UTC
Hi Emmi, thx for adopting this one!
Comment 98 Joonas Kylmälä 2020-10-21 15:53:28 UTC
Hi Emmi,

thanks for the rebases. I did code review and everything is looking really good! I have some trouble setting up my koha-testing-docker right now so I still need to go over the test plan for these patches. Could you meanwhile

a) Add your Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com> git trailer so that there will be a "paper trail" of you doing changes, now it basically reads that Lari did everything and also that the people signing-off here would have signed-off these changes but in actuality they have not. Please Add the Rebased-by trailer after the signed-off-by lines so that we know they were tested in a earlier revision and give credit for the people doing that.

b) Rebase one more time. There was minor merge conflict with one of the patches.
Comment 99 Emmi Takkinen 2020-10-22 08:11:44 UTC
Created attachment 112147 [details] [review]
Bug 18595: Unit tests for Koha::Patron::Categories->default_messaging

This patch adds unit tests for Koha::Patron::Categories->default_messaging. Tests
will help preparing for moving C4::Members::Messaging to Koha-objects.

To test:
1. Run t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 100 Emmi Takkinen 2020-10-22 08:12:39 UTC
Created attachment 112148 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingOptions

This patch moves C4::Members::GetMessagingOptions to
Koha::Patron::Message::Preferences->get_options.

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

This patch also removes previously unused GetMessagingOptions in opac-messaging.pl

To test:

Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingOptions"
   (grep -r 'GetMessagingOptions').
3. Observe no results in source code.

---- OPAC ----
1. Login to OPAC.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.

---- INTRANET ----
1. Login to intranet.
2. Go to edit patron's details and scroll down to their messaging preferences.
3. Observe the preferences looking normal and as they should be.
4. Set any combination of messaging preferences.
5. Observe them apply as expected, repeat step 4-5 as much as you like.
6. Go to Administration -> Patron categories -> Edit (for any category)
7. Scroll down to observe default messaging preferences for this patron category
8. Observe the preferences looking normal and as they should be.
9. Set any combination of messaging preferences.
10. Observe them apply as expected, repeat step 9-10 as much as you like.
11. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
12. Fill in required data, do not modify messaging preferences, click Save.
13. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 101 Emmi Takkinen 2020-10-22 08:13:36 UTC
Created attachment 112149 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreferencesFromDefaults

This patch moves C4::Members::SetMessagingPreferencesFromDefaults to
Koha::Patron->set_default_messaging_preferences

The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreferencesFromDefaults"
       (grep -r 'SetMessagingPreferencesFromDefaults').
3. Observe no results in source code.

---- INTRANET ----
1. Login to intranet.
2. Go to Administration -> Patron categories -> Edit (for any category)
3. Scroll down to observe default messaging preferences for this patron category
4. Observe the preferences, remember them for the next step.
5. Go to cgi-bin/koha/members/memberentry.pl?op=add&categorycode=XXX, replace XXX
    with the categorycode you were just editing.
6. Look at messaging preferences. They should be same as in step 4.
7. Fill in required data, do not modify messaging preferences, click Save.
8. Observe a new patron with default messaging preferences.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 102 Emmi Takkinen 2020-10-22 08:14:50 UTC
Created attachment 112150 [details] [review]
Bug 18595: Unit tests preparing for GetMessagingPreferences move

This patch prepares removing GetMessagingPreferences by adding some useful unit
tests for subroutines that the move targets.

Reserves.t already covers print notice, but this patch adds tests for a set
transport type.

To test:
1. Run t/db_dependent/Circulation.t
2. Run t/db_dependent/Reserves.t
3. Observe test pass

Works together with followup patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 103 Emmi Takkinen 2020-10-22 08:16:01 UTC
Created attachment 112151 [details] [review]
Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences

This patch moves C4::Members::Messaging::GetMessagingPreferences to
Koha::Patron::Message::Preference(s)
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "GetMessagingPreferences"
       (grep -r 'GetMessagingPreferences').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 104 Emmi Takkinen 2020-10-22 08:17:01 UTC
Created attachment 112152 [details] [review]
Bug 18595: Move C4::Members::Messaging::SetMessagingPreference

This patch moves C4::Members::Messaging::SetMessagingPreference to
Koha::Patron::Message::Preference
The patch makes a heavy touch on core messaging preference features,
but should not introduce any visible changes for end user. Keep this in mind
if testing. If you notice any changes in Koha, something is probably wrong!

To test:

Make sure you have enabled the following system preferences:
- EnhancedMessagingPreferences

---- GENERAL ----
1. Apply patch
2. Search Koha source code for references of "SetMessagingPreference"
       (grep -r 'SetMessagingPreference').
3. Observe no results in source code.

---- UNIT TESTS ----
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Works as expected. QA tools fails with:
FAIL	C4/Members/Messaging.pm pod empty section in previous paragraph
Can be fixed in a followup.
Signed-off-by: Marc Véron <veron@veron.ch>

QA tools pass now OK
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 105 Emmi Takkinen 2020-10-22 08:17:47 UTC
Created attachment 112153 [details] [review]
Bug 18595: Remove references to C4::Members::Messaging

To test:
1. grep -r 'C4::Members::Messaging'
   This command should not produce any results in Koha source code after this
   patch.
2. Check the pages touched by this commit
3. Try setting and viewing patron and patron category messaging preferences

As this completes the move from C4::Members::Messaging to new Koha-objects for
messaging preferences, everything should be ready for proper testing.

Make sure you have EnhancedMessagingPreferences system preference enabled.
Then, try creating new categories and setting default messaging preferences,
as well as editing the messaging preferences of the already existing categories.
Try adding new patrons and make sure the default patron messaging preferences
are by default the same that you set for that category. Edit already exisiting
patrons' messaging preferences and make sure changes are applied. Observe
no crash at any point, otherwise fail this patch. Don't forget to test both
intranet and OPAC.

Finally, run unit tests:
1. prove t/db_dependent/Circulation.t
2. prove t/db_dependent/Reserves.t
3. prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 106 Emmi Takkinen 2020-10-22 08:18:32 UTC
Created attachment 112154 [details] [review]
Bug 18595: Disable digest checkbox when forced on

To test:
1. Force a digest on via message_transports table
   (delete rows where message_attribute is 1 and is_digest is 0)
SQL: delete from message_transports where message_attribute_id=1 and is_digest=0;
2. Go to either OPAC or staff client messaging preferences
3. Observe digest checkbox is disabled for message attribute 1
   (normally Item due row)

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 107 Emmi Takkinen 2020-10-22 08:19:18 UTC
Created attachment 112155 [details] [review]
Bug 18595: Validate days_in_advance

Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 108 Emmi Takkinen 2020-10-22 08:23:09 UTC
(In reply to Joonas Kylmälä from comment #98)
> Hi Emmi,
> 
> thanks for the rebases. I did code review and everything is looking really
> good! I have some trouble setting up my koha-testing-docker right now so I
> still need to go over the test plan for these patches. Could you meanwhile
> 
> a) Add your Rebased-by: Emmi Takkinen <emmi.takkinen@outlook.com> git
> trailer so that there will be a "paper trail" of you doing changes, now it
> basically reads that Lari did everything and also that the people
> signing-off here would have signed-off these changes but in actuality they
> have not. Please Add the Rebased-by trailer after the signed-off-by lines so
> that we know they were tested in a earlier revision and give credit for the
> people doing that.
> 
> b) Rebase one more time. There was minor merge conflict with one of the
> patches.

Rebased and fixed minor merge conflict with tests. Added git trailer after sign-offs (followed instruction here: https://wiki.koha-community.org/wiki/Commit_messages)
Comment 109 Joonas Kylmälä 2020-10-22 14:48:26 UTC
Some things need a bit more work:

If no patron email we get error
===============================

1. Add patron without email
2. Go to cgi-bin/koha/opac-messaging.pl and select email checkbox for every messaging setting
3. Observe following error:

> Patron has not set email address, cannot use email as message transport at /usr/share/perl5/Exception/Class/Base.pm line 88
> 1. in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88
> 2. in Koha::Patron::Message::Preference::_set_message_transport_types at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 409
> 3. in Koha::Patron::Message::Preference::message_transport_types at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 224
> 4. in Koha::Patron::Message::Preference::set at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 247
> 5. in C4::Form::MessagingPreferences::handle_form_action at /kohadevbox/koha/C4/Form/MessagingPreferences.pm line 110
> 6. in (eval) at /kohadevbox/koha/opac/opac-messaging.pl line 67

After fixing this it should be made sure also that removing email address in a patron which had the messaging preferences removes the messaging preferences (or handles it some proper way).

More robust input handling in OPAC needed
=========================================

In the opac messaging preferences change with the HTML dev console the value of "Days in advance" to be 50 and submit the request, you will then get following error:

> days_in_advance has to be a value between 0-30 for Advance_Notice. at /usr/share/perl5/Exception/Class/Base.pm line 88

Expected outcome: a nicer result is shown to the user about invalid parameters, we should handle all errors in the code.

What do the last two commits do
===============================

Bug 18595: Disable digest checkbox when forced on:
 - This one maybe I don't understand because of the first error caused by missing email. Specifically I don't understand in which situation the digest checkbox should be disabled.


Bug 18595: Validate days_in_advance
 - What does this do exactly? I see it change days_in_advance=NULL but what is the reason behind that?

QA tool errors
==============

$ koha-qa.pl -v 2 -c 9

 FAIL	C4/Reserves.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   OK	  valid

 FAIL	circ/returns.pl
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/messaging-preference-form.inc
   OK	  filters
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   FAIL	  tt_valid
		lines 163, 169
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
   OK	  filters
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   FAIL	  tt_valid
		lines 131, 133
   OK	  valid_template

For the critic qa errors see the details with for example using 

$ perlcritic circ/returns.pl
$ perlcritic C4/Reserves.pm

Not quite sure how to fix the template issues.

That's all the issues I spotted. The code review I did didn't find any issues and all the test mentioned in the test plan work.
Comment 110 Lari Taskula 2020-10-28 02:12:22 UTC
(In reply to Joonas Kylmälä from comment #109)
> What do the last two commits do
> ===============================
> 
> Bug 18595: Disable digest checkbox when forced on:
>  - This one maybe I don't understand because of the first error caused by
> missing email. Specifically I don't understand in which situation the digest
> checkbox should be disabled.
There are libraries that wish to only send notifications as digests. IIRC, the only way to achieve this was to configure it directly in the database via "message_transports" table by removing rows where "is_digest" is 0. This patch disables digest checkbox elements in such cases.

> Bug 18595: Validate days_in_advance
>  - What does this do exactly? I see it change days_in_advance=NULL but what
> is the reason behind that?
It's been 3 years since I provided these patches so I can't exactly remember why and where but some patron messaging preferences had invalid values for borrower_message_preferences.days_in_advance column and it caused plenty of "Internal Server Error" messages in production. No idea whether it came from Koha bugs at some point in the history, or from sysadmins themselves, but I suppose it's safer to do a little database cleanup and run such query because it simply nulls days_in_advance for every option that does not allow it.
Comment 111 Emmi Takkinen 2020-10-28 09:28:32 UTC
(In reply to Joonas Kylmälä from comment #109)
> Some things need a bit more work:
> 
> If no patron email we get error
> ===============================
> 
> 1. Add patron without email
> 2. Go to cgi-bin/koha/opac-messaging.pl and select email checkbox for every
> messaging setting
> 3. Observe following error:
> 
> > Patron has not set email address, cannot use email as message transport at /usr/share/perl5/Exception/Class/Base.pm line 88
> > 1. in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88
> > 2. in Koha::Patron::Message::Preference::_set_message_transport_types at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 409
> > 3. in Koha::Patron::Message::Preference::message_transport_types at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 224
> > 4. in Koha::Patron::Message::Preference::set at /kohadevbox/koha/Koha/Patron/Message/Preference.pm line 247
> > 5. in C4::Form::MessagingPreferences::handle_form_action at /kohadevbox/koha/C4/Form/MessagingPreferences.pm line 110
> > 6. in (eval) at /kohadevbox/koha/opac/opac-messaging.pl line 67
> 
> After fixing this it should be made sure also that removing email address in
> a patron which had the messaging preferences removes the messaging
> preferences (or handles it some proper way).
> 
Same error occurs in the staff client if we have email as message transport type for patron (I'm guessing that error will also be raised if sms number isn't set but is chosen as message preference). However patron is still saved. 

I'm not sure if automatically removing message preference is a good approach. Maybe preventing form from sending and error message like 'Patron has email as message transport type but not email. Either provide valid email or change patrons messaging preferences.' is enough. Hmm, or error message next to input field.

> More robust input handling in OPAC needed
> =========================================
> 
> In the opac messaging preferences change with the HTML dev console the value
> of "Days in advance" to be 50 and submit the request, you will then get
> following error:
> 
> > days_in_advance has to be a value between 0-30 for Advance_Notice. at /usr/share/perl5/Exception/Class/Base.pm line 88
> 
> Expected outcome: a nicer result is shown to the user about invalid
> parameters, we should handle all errors in the code.
> 
Again same occurs in the staff client. All in all we need better error handling with these changes.

> What do the last two commits do
> ===============================
> 
> Bug 18595: Disable digest checkbox when forced on:
>  - This one maybe I don't understand because of the first error caused by
> missing email. Specifically I don't understand in which situation the digest
> checkbox should be disabled.
> 
> 
> Bug 18595: Validate days_in_advance
>  - What does this do exactly? I see it change days_in_advance=NULL but what
> is the reason behind that?
> 
Lari already commented these. With last one I agree some cleanup might be needed. First one I understand but I think it's worth checking other options... Could we e.g. disable checkbox with jQuery?

> QA tool errors
> ==============
> 
> $ koha-qa.pl -v 2 -c 9
> 
>  FAIL	C4/Reserves.pm
>    FAIL	  critic
> 		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  
>    OK	  forbidden patterns
>    OK	  git manipulation
>    OK	  pod
>    OK	  pod coverage
>    OK	  spelling
>    OK	  valid
> 
>  FAIL	circ/returns.pl
>    FAIL	  critic
> 		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  
>    OK	  forbidden patterns
>    OK	  git manipulation
>    OK	  pod
>    OK	  spelling
>    OK	  valid
> 
>  FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/messaging-preference-form.inc
>    OK	  filters
>    OK	  forbidden patterns
>    OK	  git manipulation
>    OK	  js_in_body
>    OK	  spelling
>    FAIL	  tt_valid
> 		lines 163, 169
>    OK	  valid_template
> 
>  FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
>    OK	  filters
>    OK	  forbidden patterns
>    OK	  git manipulation
>    OK	  js_in_body
>    OK	  spelling
>    FAIL	  tt_valid
> 		lines 131, 133
>    OK	  valid_template
> 
> For the critic qa errors see the details with for example using 
> 
> $ perlcritic circ/returns.pl
> $ perlcritic C4/Reserves.pm
> 
> Not quite sure how to fix the template issues.
> 
My guess is that if statement isn't allowed to be used inside input tag.
Comment 112 Lari Taskula 2020-10-30 08:36:22 UTC
After discussing with Emmi, I will be taking over this Bug once again and fix the issues mentioned above.
Comment 113 Lari Taskula 2020-10-30 11:35:49 UTC Comment hidden (obsolete)
Comment 114 Lari Taskula 2020-10-30 12:09:54 UTC
(In reply to Joonas Kylmälä from comment #109)
> More robust input handling in OPAC needed
> =========================================
> 
> In the opac messaging preferences change with the HTML dev console the value
> of "Days in advance" to be 50 and submit the request, you will then get
> following error:
> 
> > days_in_advance has to be a value between 0-30 for Advance_Notice. at /usr/share/perl5/Exception/Class/Base.pm line 88
> 
> Expected outcome: a nicer result is shown to the user about invalid
> parameters, we should handle all errors in the code.

This requires us to revisit Bug 17499. We need more detailed Koha::Exceptions to have translatable error messages in the GUI. Because now we throw multiple Koha::Exceptions::BadParameter and specifying the problem in its 'error' field. While we could parse the error message itself, I don't think that's a good idea.

Instead we would throw something like Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange with 'min' and 'max' parameters.

The more formal the exceptions the better.
Comment 115 Lari Taskula 2020-11-05 17:33:13 UTC
Created attachment 113126 [details] [review]
Bug 18595: (follow-up) Catch message preference exceptions in OPAC

This patch handles message preference exceptions in OPAC messaging template
instead of the usual white "Internal Server Error" screen.

To test:
0. This test plan assumes you have a default Koha installation that adds
   message attributes and transports from
   installer/data/mysql/mandatory/sample_notices_message_attributes.sql and
   installer/data/mysql/mandatory/sample_notices_message_transports.sql

1. Open developer tools, select console and run the following command:

$('#email1').val('non-existing message transport type').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
Message transport type non-existing message transport type does not exist.

2. Open developer tools, select console and run the following command:

$($('select[name="2-DAYS"] option')[0]).attr('value', 31);
$('form[name=opacmessaging]').submit();

Observe error:
Days in advance selection is out of range. The value must be between 0 and 30.

3. Run SQL command:

delete from message_transports where message_attribute_id=1 and is_digest=0 \
and message_transport_type='email';

Open developer tools, select console and run the following command:

$('#digest1').prop('checked', false);
$('#forcedigest1').remove();
$('form[name=opacmessaging]').submit();

Observe error:
Digest must be selected for Item_Due.

Recover your test database by running the following SQL command:

insert into message_transports (message_attribute_id, message_transport_type, \
is_digest, letter_module, letter_code) values (1, 'email', 0, 'circulation', \
'DUE');

4. Remove your email address, open developer tools, select console and run the
following command:

$('#email1').removeAttr('disabled');
$('#email1').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
You cannot select email notifications because you haven't set an email address.

5. Remove your SMS number, open developer tools, select console and run the
following command:

$('#sms1').removeAttr('disabled');
$('#sms1').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
You cannot select SMS notifications because you haven't set an SMS number address.

6. The rest of the template error messages are currently inaccessible and
therefore untestable via GUI, but check the changes this patch introduces
manually. These exceptions are:

Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable

You can test them programmatically by searching opac-messaging.pl for the
following:

C4::Form::MessagingPreferences::handle_form_action( ...

and throwing each exception before it by following code:

Koha::Exceptions::Patron::Message::Preference::AttributeNotFound->throw();

Sponsored-by: The National Library of Finland
Comment 116 Lari Taskula 2020-11-05 20:18:03 UTC
Created attachment 113192 [details] [review]
Bug 18595: (follow-up) Disable transports if missing contact information

To test:
Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC
- SMSSendDriver

---- GENERAL ----
1. Apply patch

---- PREREQUISITES ----
1. Select a patron to test this patch with
2. Remove patron's primary email
3. Remove patron's SMS number

---- OPAC ----
1. Login to OPAC with your test patron.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe two warnings "Your user account is missing email address" and
   "Your user account is missing SMS number"
4. See that the SMS and Email checkboxes are disabled in the messaging
   preferences table.
5. At the bottom of the page, set SMS number
6. Refresh page, observe only one warning "Your user account is missing
   email address" and observe the SMS checkboxes are now clickable.
7. Give your test patron a primary email address
8. Refresh OPAC "your messaging" page
9. Observe all warnings are now gone and both SMS number and Email checkboxes
   in the messaging preferences table are clickable.
10. Set patron some messaging preferences and make sure your change applies.

---- INTRANET ----
1. Repeat "OPAC" steps but instead of "your messaging" page, use the
   "Modify patron" view.

Sponsored-by: The National Library of Finland
Comment 117 Lari Taskula 2020-11-06 09:56:57 UTC
Created attachment 113221 [details] [review]
Bug 18595: (follow-up) Disable transports if missing contact information

To test:
Make sure you have enabled the following system preferences:
- opacuserlogin
- EnhancedMessagingPreferences
- EnhancedMessagingPreferencesOPAC
- SMSSendDriver

---- GENERAL ----
1. Apply patch

---- PREREQUISITES ----
1. Select a patron to test this patch with
2. Remove patron's primary email
3. Remove patron's SMS number

---- OPAC ----
1. Login to OPAC with your test patron.
2. Go to "your messaging" (cgi-bin/koha/opac-messaging.pl).
3. Observe two warnings "Cannot select email notifications" and
   "Cannot select SMS notifications"
4. See that the SMS and Email checkboxes are disabled in the messaging
   preferences table.
5. At the bottom of the page, set SMS number
6. Refresh page, observe only one warning "Cannot select email notifications"
   and observe the SMS checkboxes are now clickable.
7. Give your test patron a primary email address
8. Refresh OPAC "your messaging" page
9. Observe all warnings are now gone and both SMS number and Email checkboxes
   in the messaging preferences table are clickable.
10. Set patron some messaging preferences and make sure your change applies.

---- INTRANET ----
1. Repeat "OPAC" steps but instead of "your messaging" page, use the
   "Modify patron" view.

Sponsored-by: The National Library of Finland
Comment 118 Lari Taskula 2020-11-06 09:57:02 UTC
Created attachment 113222 [details] [review]
Bug 18595: (follow-up) Catch message preference exceptions in OPAC

This patch handles message preference exceptions in OPAC messaging template
instead of the usual white "Internal Server Error" screen.

To test:
0. This test plan assumes you have a default Koha installation that adds
   message attributes and transports from
   installer/data/mysql/mandatory/sample_notices_message_attributes.sql and
   installer/data/mysql/mandatory/sample_notices_message_transports.sql

1. Open developer tools, select console and run the following command:

$('#email1').val('non-existing message transport type').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
Message transport type non-existing message transport type does not exist.

2. Open developer tools, select console and run the following command:

$($('select[name="2-DAYS"] option')[0]).attr('value', 31);
$('form[name=opacmessaging]').submit();

Observe error:
Days in advance selection is out of range. The value must be between 0 and 30.

3. Run SQL command:

delete from message_transports where message_attribute_id=1 and is_digest=0 \
and message_transport_type='email';

Open developer tools, select console and run the following command:

$('#digest1').prop('checked', false);
$('#forcedigest1').remove();
$('form[name=opacmessaging]').submit();

Observe error:
Digest must be selected for Item_Due.

Recover your test database by running the following SQL command:

insert into message_transports (message_attribute_id, message_transport_type, \
is_digest, letter_module, letter_code) values (1, 'email', 0, 'circulation', \
'DUE');

4. Remove your email address, open developer tools, select console and run the
following command:

$('#email1').removeAttr('disabled');
$('#email1').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
You cannot select email notifications because you haven't set an email address.

5. Remove your SMS number, open developer tools, select console and run the
following command:

$('#sms1').removeAttr('disabled');
$('#sms1').prop('checked', true);
$('form[name=opacmessaging]').submit();

Observe error:
You cannot select SMS notifications because you haven't set an SMS number address.

6. The rest of the template error messages are currently inaccessible and
therefore untestable via GUI, but check the changes this patch introduces
manually. These exceptions are:

Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable

You can test them programmatically by searching opac-messaging.pl for the
following:

C4::Form::MessagingPreferences::handle_form_action( ...

and throwing each exception before it by following code:

Koha::Exceptions::Patron::Message::Preference::AttributeNotFound->throw();

Sponsored-by: The National Library of Finland
Comment 119 Lari Taskula 2020-11-18 04:30:06 UTC
Created attachment 113769 [details] [review]
Bug 18595: (follow-up) Remove duplicated logic

Sponsored-by: The National Library of Finland