Simith, please remember to assign patches to yourself :)

I really like this, as I have been asked to hide the message settings quite often and right now there is no solution the user can't get around. 

But there is a problem with this patch there: it just deactivates the visibility of the tab in the templates, but it doesn't block accessing the message settings page if you know the URL. I know we have fixed that for other pages in patron account, could you take a look and try to fix this? 

Database update looks good, the new condition could be added to the existing [%IF ...%], but not a blocker.

Fix done, Url access blocked

Hi Simith

As your first patch was already signed off, I reformatted your second to only include the new changes and signed off on that.

I am sorry, those patches look ok, but the database update is missing.

The description of EnhancedMessagingPreferences should probably be reworded as it does not make much sense in combination with OPACEnhancedMessagingPreferences.

Comment on attachment 36512 [details] [review]
Bug 12528 - Enable staff to deny message setting access to patrons on the OPAC

Review of attachment 36512 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-messaging.pl
@@ +69,5 @@
>                    SMSnumber => defined $borrower->{'smsalertnumber'} ? $borrower->{'smsalertnumber'} : $borrower->{'mobile'},
>                    SMSSendDriver                =>  C4::Context->preference("SMSSendDriver"),
> +                  TalkingTechItivaPhone        =>  C4::Context->preference("TalkingTechItivaPhoneNotification"),
> +                  admin                        =>  C4::Context->preference('KohaAdminEmailAddress'),
> +                  Authorized                   =>  C4::Context->preference('OPACEnhancedMessagingPreferences'));

Please use the Koha plugin to access system preference values from the templates ([% IF Koha.Preference('OPACEnhancedMessagingPreferences') %]).
And maybe EnhancedMessagingPreferences should be checked too, according to the description of the syspref.

Patch does not apply:


Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Failed to merge in the changes.

I redone all patch because it's necessary, I create one good patch work correctly.

In opac-messaging.pl, you should remove 'Authorized' parameter, and use in
opac-messaging.tt:

[% IF Koha.Preference( 'OPACEnhancedMessagingPreferences' ) %]

I removed 'Authorized' parameter in opac-messaging.pl
and used  in opac-messaging.tt  [% IF Koha.Preference( 'OPACEnhancedMessagingPreferences' ) %].

(In reply to Bouzid from comment #23)
> I removed 'Authorized' parameter in opac-messaging.pl
> and used  in opac-messaging.tt  [% IF Koha.Preference(
> 'OPACEnhancedMessagingPreferences' ) %].

Thanks. The code seems OK now.

I realize that the wording of the 2 sysprefs are not that clear. The two
together. And it's not your syspref which sound strange bug the initial:

  EnhancedMessagingPreferences: Allow/Don't allow patrons to choose which
  notices they receive and when they receive them. Note that this only applies
  to certain kinds of notices.

This give the feeling that the patron will be allowed to change their
notifications themselves.

Anyway your patch works.

Comment on attachment 44826 [details] [review]
Bug 12528 - Enable staff to deny message setting access to patrons on the OPAC

Review of attachment 44826 [details] [review]:
-----------------------------------------------------------------

The description for EnhancedMessagingPreferences is "allow/don't allow patrons to choose which notices they receive and when they receive them.
I think it could be updated to highlight that it's for the staff interface only, or at least a reference to the new pref?

Why don't you want the patrons to show the type of messages they receive?
I could understand not to let the patrons change the settings, but I don't see the point to hide them completely.

::: installer/data/mysql/atomicupdate/sysprefs_Bug12528.sql
@@ +1,3 @@
> +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> +('OPACEnhancedMessagingPreferences', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo')
> +;

It would be better to set the value of the other pref, to avoid any confusion.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
@@ +131,5 @@
>                      </form>
>                  </div> <!-- / #usermessaging -->
> +                [% ELSE %]
> +                    <h4>You are not allowed to call this page directly</h4>
> +                    <span> To report this problem, you can <a href="mailto:[% admin %]">email the Koha Administrator</a>.</span>

"admin" does not seem to be sent to the template.

(In reply to Jonathan Druart from comment #26)
> Comment on attachment 44826 [details] [review] [review]
> Bug 12528 - Enable staff to deny message setting access to patrons on the
> OPAC
> 
> Review of attachment 44826 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> The description for EnhancedMessagingPreferences is "allow/don't allow
> patrons to choose which notices they receive and when they receive them.
> I think it could be updated to highlight that it's for the staff interface
> only, or at least a reference to the new pref?

You're right. We will change the preference description.
It will be :

[Allow/Don't allow] staff to manage which notices patrons will receive and when they will receive them. Note : this only applies to courtesy notices. To manage if patrons have also access to these settings, use OPACEnhancedMessagingPreferences.


> Why don't you want the patrons to show the type of messages they receive?
> I could understand not to let the patrons change the settings, but I don't
> see the point to hide them completely.

By hiding them, you avoid the question : "Why can't I change my settings by myself?" ;^)

> ::: installer/data/mysql/atomicupdate/sysprefs_Bug12528.sql
> @@ +1,3 @@
> > +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> > +('OPACEnhancedMessagingPreferences', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo')
> > +;
> 
> It would be better to set the value of the other pref, to avoid any
> confusion.

EnhancedMessagingPreference is already setup by updatedatabase so I don't think this should be change by the atomicupdate here. Moreover, we don't want to change the current setting when the library will run this update.

> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
> @@ +131,5 @@
> >                      </form>
> >                  </div> <!-- / #usermessaging -->
> > +                [% ELSE %]
> > +                    <h4>You are not allowed to call this page directly</h4>
> > +                    <span> To report this problem, you can <a href="mailto:[% admin %]">email the Koha Administrator</a>.</span>
> 
> "admin" does not seem to be sent to the template.

We are currently working on this. :^)

(In reply to Francois Charbonnier from comment #27)
> > Why don't you want the patrons to show the type of messages they receive?
> > I could understand not to let the patrons change the settings, but I don't
> > see the point to hide them completely.
> 
> By hiding them, you avoid the question : "Why can't I change my settings by
> myself?" ;^)

But can answer the question "Why am I receiving email for that?"
We can imagine a quick sentence "Contact your library if you want to change them"

> > ::: installer/data/mysql/atomicupdate/sysprefs_Bug12528.sql
> > @@ +1,3 @@
> > > +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> > > +('OPACEnhancedMessagingPreferences', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo')
> > > +;
> > 
> > It would be better to set the value of the other pref, to avoid any
> > confusion.
> 
> EnhancedMessagingPreference is already setup by updatedatabase so I don't
> think this should be change by the atomicupdate here. Moreover, we don't
> want to change the current setting when the library will run this update.

OPACEnhancedMessagingPreferences is added (for existing install, so in updatedatabase.pl) and enabled.
It does not hurt if EnhancedMessagingPreference is disabled, because in the code both are checked.
But it would be better to enable OPACEnhancedMessagingPreferences only if EnhancedMessagingPreference is enabled too.
It will avoid to have
"OPACEnhancedMessagingPreference is enabled but not really because EnhancedMessagingPreference is not" on the admin pref page.

Suggestion for OPACEnhancedMessagingPreferences. It could have 3 values rather
than 2:

  0- Not displayed
  1- Displayed
  2- Modifiable 

When updating an existing DB (see Jonathan comment):

  my $emp = C4::Context->preference('EnhancedMessagingPreferences')
            ? 2 : 0;
  INSERT INTO systempreferences (variable, value, options, explanation, type) VALUES
  ('OPACEnhancedMessagingPreferences', $emp, NULL,
    'Not display/display/modify patrons messaging setting on the OPAC.', 'YesNo')

For the sake of better translatability (no string spltting by html tags), I propose to change
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt line 135:

<span> To report this problem, you can <a href="mailto:[% admin %]">email the Koha Administrator</a>.</span>

... to something like:

<span> To report this problem, please contact the Koha Administrator: <a href="mailto:[% admin %]">Email.</a></span>

(In reply to Jonathan Druart from comment #28)
> > > ::: installer/data/mysql/atomicupdate/sysprefs_Bug12528.sql
> > > @@ +1,3 @@
> > > > +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> > > > +('OPACEnhancedMessagingPreferences', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo')
> > > > +;
> > > 
> > > It would be better to set the value of the other pref, to avoid any
> > > confusion.
> > 
> > EnhancedMessagingPreference is already setup by updatedatabase so I don't
> > think this should be change by the atomicupdate here. Moreover, we don't
> > want to change the current setting when the library will run this update.
> 
> OPACEnhancedMessagingPreferences is added (for existing install, so in
> updatedatabase.pl) and enabled.
> It does not hurt if EnhancedMessagingPreference is disabled, because in the
> code both are checked.
> But it would be better to enable OPACEnhancedMessagingPreferences only if
> EnhancedMessagingPreference is enabled too.
> It will avoid to have
> "OPACEnhancedMessagingPreference is enabled but not really because
> EnhancedMessagingPreference is not" on the admin pref page.

I agree. We will change the atomic update to enable the OPACEnhancedMessagingPreferences only if EnhancedMessagingPreference is enable as well.

(In reply to Jonathan Druart from comment #28)
> (In reply to Francois Charbonnier from comment #27)
> > > Why don't you want the patrons to show the type of messages they receive?
> > > I could understand not to let the patrons change the settings, but I don't
> > > see the point to hide them completely.
> > 
> > By hiding them, you avoid the question : "Why can't I change my settings by
> > myself?" ;^)
> 
> But can answer the question "Why am I receiving email for that?"
> We can imagine a quick sentence "Contact your library if you want to change
> them"

I like Frederic's option but we will not do it. :^) If anyone want his third option, I'll suggest to open a new ticket for the enhancement.

My thoughts on this are : this patch does not change the current behaviour if not activated. For libraries who can see how useful it is (like the one who sponsored it, use it for more than a year now and hasn't got any complaint so far), it's there. For libraries who doesn't see it, they just don't activate it. Everybody's happy! :^)

In the end, I'll just take the opportunity to say that I know how developers don't like new system preferences (I understand their points) but us, librarians, do like them a lot. They allow us to be even more in control of our ILS which I think is fantastic. We can all agree Koha is all about empowering librarians with new features, right? But, the difficulty is to find the better way do it. I think we should always see it as a step by step process.

This patch is the first step for a better courtesy notices management. Once this patch is pushed, it will benefit the libraries who see its usefullness and change nothing for libraries who can't see it. AND it will let anybody who want to enhanced it to go further.

So please, lets not abandoned this patch in the small enhancement patches limbo of bugzilla because we want to move two steps at a time. Let's do it step by step! 

Thanks!!! :^)

No idea where this is at now (even after reading most of the comments), but it's exciting nonetheless!

I usually hide these using jQuery, which isn't really the best solution, so it would be great to have a system preference for this...

(In reply to Francois Charbonnier from comment #32)
(...) 
> So please, lets not abandoned this patch in the small enhancement patches
> limbo of bugzilla because we want to move two steps at a time. Let's do it
> step by step! 

Step by step ++

(In reply to Francois Charbonnier from comment #32)

> So please, lets not abandoned this patch in the small enhancement patches
> limbo of bugzilla because we want to move two steps at a time. Let's do it
> step by step! 

I agree. You have to strike while the iron is hot. I'd be pleased to re-sign-off this patch as soon as issue rightly raised by Jonathan will be addressed.

(In reply to Frédéric Demians from comment #35)
> (In reply to Francois Charbonnier from comment #32)
> 
> > So please, lets not abandoned this patch in the small enhancement patches
> > limbo of bugzilla because we want to move two steps at a time. Let's do it
> > step by step! 
> 
> I agree. You have to strike while the iron is hot. I'd be pleased to
> re-sign-off this patch as soon as issue rightly raised by Jonathan will be
> addressed.

And I will be happy to re-qa it ;)

Comment on attachment 45962 [details] [review]
Bug 12528 - Enable staff to deny message setting access to patrons on the OPAC

Review of attachment 45962 [details] [review]:
-----------------------------------------------------------------

Please provide a quick fix, it's easy :)

::: installer/data/mysql/atomicupdate/sysprefs_Bug12528.sql
@@ +1,4 @@
> +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> +('OPACEnhancedMessagingPreferences', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo') ON DUPLICATE KEY UPDATE value ='1';
> +INSERT INTO systempreferences ( variable, value, options, explanation,type ) VALUES
> +('EnhancedMessagingPreferences', '1', NULL, 'If ON, allows patrons to select to receive additional messages about items due or nearly due.', 'YesNo') ON DUPLICATE KEY UPDATE value ='1';

Why do you want to modify the values on duplicate?
You just need to insert the new pref OPACEnhancedMessagingPreferences with the same value as EnhancedMessagingPreferences.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
@@ +157,5 @@
> +         - pref: OPACEnhancedMessagingPreferences
> +           choices:
> +               yes: Show
> +               no: "Don't show"
> +         - "patron messaging setting on the OPAC (NOTE: EnhancedMessagingPreferences must be enabled)."

No need to uppercase NOTE.

Actually, this bug is a duplicate of bug 12528, sorry not to have see that before.

(In reply to Jonathan Druart from comment #43)
> Actually, this bug is a duplicate of bug 12528, sorry not to have see that
> before.

Read bug 9254.

See Kyle's followup bug 9254 comment 4

Another issue is that there is no check in the opac/opac-messaging.pl script to forbid the user to modify the messaging changes, see my quick patch on bug 9254 comment 16.

The correct way to continue here would be to mark this one as a duplicate of bug 9254 and work on it.

(In reply to Bouzid from comment #47)

Please answer before switching back to Needs Signoff.

Hi Jonathan,
We should work on this patch because we have someone to work on it right now.
The other one will be an enhancement of this one.
Thanks et bonne année! ;)

(In reply to Francois Charbonnier from comment #49)
> Hi Jonathan,
> We should work on this patch because we have someone to work on it right now.
> The other one will be an enhancement of this one.
> Thanks et bonne année! ;)

Have you read the comments on bug 9254 I have pointed out?
It's not an enhancement, both bug reports have the same goal.
There is no need to ask for a signoff, it will fail QA.

Howdy !

> There is no need to ask for a signoff, it will fail QA.
The latest commit by Bouzid fixed the problem from accessing the page when disabled (BTW, I asked him to change the code to have a single IF / ELSE statement in the .tt and change the pref name for EnhancedMessagingPreferencesOPAC which he will commit soon).

Concerning bug 9254, it basically add another option to the EnhancedMessagingPreferencesOPAC, so, from my understanding it's not quite a duplicate, but an enhancement of this one, as Francois pointed out.

@Jonathan : Just want to check with you that this bug will move forward if we fix the behaviour when accessing the URL and changing the pref name.

Cheers !

(In reply to Eric Bégin from comment #51)
> Howdy !
> 
> > There is no need to ask for a signoff, it will fail QA.
> The latest commit by Bouzid fixed the problem from accessing the page when
> disabled (BTW, I asked him to change the code to have a single IF / ELSE
> statement in the .tt and change the pref name for
> EnhancedMessagingPreferencesOPAC which he will commit soon).

No, it's not. Please see, read and follow link on comment 46:
"Another issue is that there is no check in the opac/opac-messaging.pl script to forbid the user to modify the messaging changes, see my quick patch on bug 9254 comment 16."
It's not because a page is not accessible that a user cannot call it and pass parameters via POST or GET.

> Concerning bug 9254, it basically add another option to the
> EnhancedMessagingPreferencesOPAC, so, from my understanding it's not quite a
> duplicate, but an enhancement of this one, as Francois pointed out.

We already discussed about that here, and we all on that.
The discussions are exactly the same on both bug report, that's why I said it's a duplicate.

> @Jonathan : Just want to check with you that this bug will move forward if
> we fix the behaviour when accessing the URL and changing the pref name.

Yes.

Please keep authorship

Does not apply:

Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/sysprefs.sql
Applying: Bug 12528 - fix problem when insert OPACEnhancedMessagingPreferences
Applying: Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC
fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC

Bouzid, why did you want me to rebase this patchset?

Jonathan,
I rebase you this patchset, because you are author of --Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC--, when apply this patch, it fail at this, it show a message:
    Applying: Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC
fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC
Bouzid.

(In reply to Bouzid from comment #59)
> Jonathan,
> I rebase you this patchset, because you are author of --Bug 12528: Bug 9254:
> Followup - Rename pref to EnhancedMessagingPreferencesOPAC

I am not, Kyle is the author of this patch.
Rebased patches are coming.

Created attachment 51000 [details] [review]
Bug 12528 - Enable staff to deny message setting access to patrons on the OPAC

- Change the preference Enhancedmessagingpreference description.
  - Enable default EnhancedMessagingPreferences and OPACEnhancedMessagingPreferences.
  - not sent e-mail it's necessary, when user call opac-messaging.pl directly..

Testing:

I Apply the patch
II Run updatedatabase.pl

0) Search OPACEnhancedMessagingPreferences preference;
1) Validate "OPACEnhancedMessagingPreferences show patron messaging
   setting on the OPAC (NOTE: EnhancedMessagingPreferences must be
   enabled).";

2) Disable OPACEnhancedMessagingPreferences preference;
3) Enable EnhancedMessagingPreferences preference;
4) On the OPAC -> user's settings, validate "your messaging" is not
   showed.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
  Works as expected. With the new syspref, patrons can be forbidden to
  modify themselves their own messaging preferences.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Also, I like sysprefs
http://www.quickmeme.com/img/d9/d99723bc544e8d33572dc92f242a6f6e2dbe0126a2e35fe3de073d30d62002e6.jpg

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 51001 [details] [review]
Bug 12528 - fix problem when insert OPACEnhancedMessagingPreferences

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 51002 [details] [review]
Bug 12528: Bug 9254: Followup - Rename pref to EnhancedMessagingPreferencesOPAC

If the new pref is named EnhancedMessagingPreferencesOPAC, it will show
up adjacent to EnhancedMessagingPreferences

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 51003 [details] [review]
Bug 12528: redirect to 404 if at least 1 pref is off

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 51004 [details] [review]
Bug 12528: Add IGNORE to the INSERT statement

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master for Koha 16.05, thanks Bouzid!

*** Bug 9254 has been marked as a duplicate of this bug. ***