Bug 31503 - Allow several consent types on the consents tab of OPAC account page
Summary: Allow several consent types on the consents tab of OPAC account page
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20380 31570
  Show dependency treegraph
 
Reported: 2022-09-01 07:14 UTC by Marcel de Rooy
Modified: 2022-10-11 11:06 UTC (History)
4 users (show)

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


Attachments
Bug 31503: Add $patron->consent (3.02 KB, patch)
2022-09-01 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Use patron->consent in opac-patron-consent (8.22 KB, patch)
2022-09-01 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.97 KB, patch)
2022-09-01 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.50 KB, patch)
2022-09-01 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Adjust db column patron_consent.type (1.35 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $patron->consent (3.02 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Use patron->consent in opac-patron-consent (8.22 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.97 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.55 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: (Experimental) Configurable consent text (7.00 KB, patch)
2022-09-02 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Change usermenu condition (1.30 KB, patch)
2022-09-02 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.97 KB, patch)
2022-09-02 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.55 KB, patch)
2022-09-02 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: (Experimental) Configurable consent text (7.00 KB, patch)
2022-09-02 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Adjust db column patron_consent.type (2.10 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $patron->consent (3.02 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Use patron->consent in opac-patron-consent (8.22 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Change usermenu condition (1.30 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.97 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.75 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Configurable consent text (4.23 KB, patch)
2022-09-15 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Adjust db column patron_consent.type (2.10 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $patron->consent (3.26 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Use patron->consent in opac-patron-consent (8.22 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Change usermenu condition (1.30 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.97 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.75 KB, patch)
2022-10-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Configurable consent text (4.12 KB, patch)
2022-10-03 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: Adjust db column patron_consent.type (2.11 KB, patch)
2022-10-03 14:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Add $patron->consent (3.27 KB, patch)
2022-10-03 14:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Use patron->consent in opac-patron-consent (8.23 KB, patch)
2022-10-03 14:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Change usermenu condition (1.31 KB, patch)
2022-10-03 14:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Add $av_cat->values (1.98 KB, patch)
2022-10-03 14:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Make opac-patron-consent more generic (10.75 KB, patch)
2022-10-03 14:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: Configurable consent text (4.13 KB, patch)
2022-10-03 14:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 31503: (follow-up) Check return of GetByCode (2.87 KB, patch)
2022-10-04 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31503: (QA follow-up) Fix POD warning (857 bytes, patch)
2022-10-10 11:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-09-01 07:14:03 UTC

    
Comment 1 Marcel de Rooy 2022-09-01 14:26:51 UTC
Created attachment 140047 [details] [review]
Bug 31503: Add $patron->consent

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-09-01 14:26:55 UTC
Created attachment 140048 [details] [review]
Bug 31503: Use patron->consent in opac-patron-consent

Includes a few small additional changes in template:
[1] op == save (more generic)
[2] agreed/disagreed replaced by 1/0
[3] toggling display of consent_info paragraph

Test plan:
Enable GDPR_Policy.
Test adding/removing consent. Check what happens with
alert, consent_info paragraph and save button.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-09-01 14:26:58 UTC
Created attachment 140049 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2022-09-01 14:27:02 UTC
Created attachment 140050 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
In order to simplify the alert for GDPR was moved to the description
text. Note that we could move this also to a AC HTML block at some
point.

Test plan:
First check the form without PATRON_CONSENT category defined.
Add AV category PATRON_CONSENT. Add GDPR_PROCESSING and another one
(e.g. NEWSLETTER).
Verify that you can toggle consent/dissent and that everything is
saved correctly. Note only a real change updates the timestamp.
Comment 5 Marcel de Rooy 2022-09-01 14:28:54 UTC
What I am still adding here, is probably a text block for a consent type from Additional contents.
What is here now already, allows you to define new consent types and register them on the consents page.
Comment 6 Marcel de Rooy 2022-09-02 08:02:17 UTC
Created attachment 140080 [details] [review]
Bug 31503: Adjust db column patron_consent.type

We want to control this field now via authorised values. An ENUM
is too restrictive.
A foreign key on the values of authorised_value for category
'PATRON_CONSENT' is not possible.
This will do for now.

Test plan:
Run dbrev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2022-09-02 08:02:21 UTC
Created attachment 140081 [details] [review]
Bug 31503: Add $patron->consent

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2022-09-02 08:02:24 UTC
Created attachment 140082 [details] [review]
Bug 31503: Use patron->consent in opac-patron-consent

Includes a few small additional changes in template:
[1] op == save (more generic)
[2] agreed/disagreed replaced by 1/0
[3] toggling display of consent_info paragraph

Test plan:
Enable GDPR_Policy.
Test adding/removing consent. Check what happens with
alert, consent_info paragraph and save button.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2022-09-02 08:02:28 UTC
Created attachment 140083 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2022-09-02 08:02:32 UTC
Created attachment 140084 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
In order to simplify the alert for GDPR was moved to the description
text. Note that we could move this also to a AC HTML block at some
point.

Test plan:
First check the form without PATRON_CONSENT category defined.
Add AV category PATRON_CONSENT. Add GDPR_PROCESSING and another one
(e.g. NEWSLETTER).
Verify that you can toggle consent/dissent and that everything is
saved correctly. Note only a real change updates the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2022-09-02 08:02:35 UTC
Created attachment 140085 [details] [review]
Bug 31503: (Experimental) Configurable consent text

This patch allows you to add title and content from the
Additional Contents module but it is still slightly
experimental since we depend on setting the code column
to a value that is not yet supported. Will open a report
for that.

Note however that the template contains a fallback in
case you might not be able to add contents yet.

Test plan:
[1] Add a NEWSLETTER value under AV category PATRON_CONSENT.
[2] Add a PatronConsent record under html customizations. Only
add title and content for Default.
[3] Change code via SQL (awaiting further AC development):
    update additional_contents
        set code='PatronConsent_NEWSLETTER'
    where location='PatronConsent' and idnew=[YOUR_ID]
Note: YOUR_ID refers to the record you added. After this
change, you will be able to add translations under Additional
contents. The code will be copied.
[4] Finally, check patron consent page for your text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2022-09-02 08:59:19 UTC
Created attachment 140087 [details] [review]
Bug 31503: Change usermenu condition

Test plan:
Check if you see Consents tab depending on GDPR_Policy or having
PATRON_CONSENT authorised values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-09-02 08:59:22 UTC
Created attachment 140088 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-09-02 08:59:26 UTC
Created attachment 140089 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
In order to simplify the alert for GDPR was moved to the description
text. Note that we could move this also to a AC HTML block at some
point.

Test plan:
First check the form without PATRON_CONSENT category defined.
Add AV category PATRON_CONSENT. Add GDPR_PROCESSING and another one
(e.g. NEWSLETTER).
Verify that you can toggle consent/dissent and that everything is
saved correctly. Note only a real change updates the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2022-09-02 08:59:29 UTC
Created attachment 140090 [details] [review]
Bug 31503: (Experimental) Configurable consent text

This patch allows you to add title and content from the
Additional Contents module but it is still slightly
experimental since we depend on setting the code column
to a value that is not yet supported. Will open a report
for that.

Note however that the template contains a fallback in
case you might not be able to add contents yet.

Test plan:
[1] Add a NEWSLETTER value under AV category PATRON_CONSENT.
[2] Add a PatronConsent record under html customizations. Only
add title and content for Default.
[3] Change code via SQL (awaiting further AC development):
    update additional_contents
        set code='PatronConsent_NEWSLETTER'
    where location='PatronConsent' and idnew=[YOUR_ID]
Note: YOUR_ID refers to the record you added. After this
change, you will be able to add translations under Additional
contents. The code will be copied.
[4] Finally, check patron consent page for your text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-09-02 09:10:37 UTC
(In reply to Marcel de Rooy from comment #15)

> This patch allows you to add title and content from the
> Additional Contents module but it is still slightly
> experimental since we depend on setting the code column
> to a value that is not yet supported. Will open a report
> for that.

Bug 31509

> Note however that the template contains a fallback in
> case you might not be able to add contents yet.
Comment 17 Marcel de Rooy 2022-09-08 06:44:53 UTC
The last patch needs a bit more attention on selecting the right record and could benefit from development on 29144.
Furthermore we could think about including opac-memberentry and opac-registration-verify.
But I am bit hesitant to continue here since 29144 is still stuck in PQA ;)
Comment 18 Marcel de Rooy 2022-09-15 05:48:26 UTC
Having another look here
Comment 19 Marcel de Rooy 2022-09-15 13:56:08 UTC
Created attachment 140666 [details] [review]
Bug 31503: Adjust db column patron_consent.type

We want to control this field now via authorised values. An ENUM
is too restrictive.
A foreign key on the values of authorised_value for category
'PATRON_CONSENT' is not possible.
This will do for now.

Test plan:
Run dbrev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2022-09-15 13:56:13 UTC
Created attachment 140667 [details] [review]
Bug 31503: Add $patron->consent

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2022-09-15 13:56:17 UTC
Created attachment 140668 [details] [review]
Bug 31503: Use patron->consent in opac-patron-consent

Includes a few small additional changes in template:
[1] op == save (more generic)
[2] agreed/disagreed replaced by 1/0
[3] toggling display of consent_info paragraph

Test plan:
Enable GDPR_Policy.
Test adding/removing consent. Check what happens with
alert, consent_info paragraph and save button.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2022-09-15 13:56:21 UTC
Created attachment 140669 [details] [review]
Bug 31503: Change usermenu condition

Test plan:
Check if you see Consents tab depending on GDPR_Policy or having
PATRON_CONSENT authorised values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2022-09-15 13:56:26 UTC
Created attachment 140670 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2022-09-15 13:56:30 UTC
Created attachment 140671 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
Note: the alert for GDPR was moved to the description.

Test plan:
First check the form without any PATRON_CONSENT values in AV. Toggle
the value of pref GDPR_Policy.
Now add GDPR_PROCESSING and another one (e.g. NEWSLETTER).
Refresh the OPAC form. Verify that you can toggle consent/dissent
and that everything is saved correctly. Note only a real change updates
the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2022-09-15 13:56:33 UTC
Created attachment 140672 [details] [review]
Bug 31503: Configurable consent text

Although I planned to implement this via Additional
contents (AC), I am using the AV description here after
all, since AC does not yet provide enough flexibility.

The title on the consents page is constructed from the
consent type now still, and the description is from the
OPAC description in AV (PATRON_CONSENT).

Note: AV allows you to add html in the description, although
it was not meant to do so. When we switch to AC later, we
will have the possibility of html, languages, title and
content.

Second note: We still need AV translations too. See
Bugzilla.

Test plan:
[1] Add a NEWSLETTER value under AV category PATRON_CONSENT.
[2] Add no OPAC description. Save. Hit OPAC consents page.
[3] Add description. Check page again.
[4] Add html to description. Check again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2022-09-15 13:59:41 UTC
The experimental patch with Additional Contents is obsoleted. The feature is not flexible enough yet. Will open another report to move to AC later.
Comment 27 Martin Renvoize 2022-09-20 13:09:38 UTC
This is looking like a great piece of functionality.

Couple of things to note:
1) I think I expected these new consent options to appear in the self-registration form alongside the existing gdpr processing consent.. or for something to direct the user to consent to them immediately after registration?
2) I find the exposure of the AV code a on the page a little strange.. Some AV's have been abused to use 'Staff' and 'OPAC' or 'Title' and 'Description'.. perhaps this might be a suitable resolution?

I've not dug to deeply yet.. but is there any notification going on anywhere when a consent is later removed?  I could imagine a newsletter consent being removed but that being missed by librarians?

Otherwise, I really like this enhancement.
Comment 28 Marcel de Rooy 2022-09-20 13:39:17 UTC
(In reply to Martin Renvoize from comment #27)

Thx for taking a look. Good questions.

> 1) I think I expected these new consent options to appear in the
> self-registration form alongside the existing gdpr processing consent.. or
> for something to direct the user to consent to them immediately after
> registration?

I treated GDPR as a special one (for historic reasons and when enforced, it rather aggressively asks for consent). Yes, I thought also of adding them on self reg, but later moved away from it. A future extension is allowing to specify opt-in or opt-out. In that case we should add implicit consents for opt-out when finishing a self reg. And think of the best way to communicate that to the user. Apart from GDPR the other consents are now handled as opt-in and not (yet) enforced in some way. The omnibus report also mentions a 'mandatory' type.
To keep it simple here, I did not add them.

> 2) I find the exposure of the AV code a on the page a little strange.. Some
> AV's have been abused to use 'Staff' and 'OPAC' or 'Title' and
> 'Description'.. perhaps this might be a suitable resolution?

AV was actually my fallback. I wanted it on Additional contents but we need to make that a bit more flexible before moving. So I considered this a bit as temporary measure to get it moving. We could do as you say but it is also a bit confusing..

> I've not dug to deeply yet.. but is there any notification going on anywhere
> when a consent is later removed?  I could imagine a newsletter consent being
> removed but that being missed by librarians?

This need is mentioned too on the omnibus. I was thinking of the Reporting module first here. But you could have a job that exports them etc.
If newsletter processing is in Koha, you could simply check real-time.
Comment 29 David Nind 2022-09-30 18:00:12 UTC
The patch no longer applies 8-(..

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 31503: Adjust db column patron_consent.type
Applying: Bug 31503: Add $patron->consent
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	t/db_dependent/Koha/Patron.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patron.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patron.t
Auto-merging Koha/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 31503: Add $patron->consent
Comment 30 Marcel de Rooy 2022-10-03 07:00:34 UTC
Created attachment 141216 [details] [review]
Bug 31503: Adjust db column patron_consent.type

We want to control this field now via authorised values. An ENUM
is too restrictive.
A foreign key on the values of authorised_value for category
'PATRON_CONSENT' is not possible.
This will do for now.

Test plan:
Run dbrev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2022-10-03 07:00:39 UTC
Created attachment 141217 [details] [review]
Bug 31503: Add $patron->consent

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-10-03 07:00:43 UTC
Created attachment 141218 [details] [review]
Bug 31503: Use patron->consent in opac-patron-consent

Includes a few small additional changes in template:
[1] op == save (more generic)
[2] agreed/disagreed replaced by 1/0
[3] toggling display of consent_info paragraph

Test plan:
Enable GDPR_Policy.
Test adding/removing consent. Check what happens with
alert, consent_info paragraph and save button.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-10-03 07:00:47 UTC
Created attachment 141219 [details] [review]
Bug 31503: Change usermenu condition

Test plan:
Check if you see Consents tab depending on GDPR_Policy or having
PATRON_CONSENT authorised values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2022-10-03 07:00:51 UTC
Created attachment 141220 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2022-10-03 07:00:56 UTC
Created attachment 141221 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
Note: the alert for GDPR was moved to the description.

Test plan:
First check the form without any PATRON_CONSENT values in AV. Toggle
the value of pref GDPR_Policy.
Now add GDPR_PROCESSING and another one (e.g. NEWSLETTER).
Refresh the OPAC form. Verify that you can toggle consent/dissent
and that everything is saved correctly. Note only a real change updates
the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2022-10-03 07:01:01 UTC
Created attachment 141222 [details] [review]
Bug 31503: Configurable consent text

Although I planned to implement this via Additional
contents (AC), I am using the AV description here after
all, since AC does not yet provide enough flexibility.

The title on the consents page is constructed from the
consent type now still, and the description is from the
OPAC description in AV (PATRON_CONSENT).

Note: AV allows you to add html in the description, although
it was not meant to do so. When we switch to AC later, we
will have the possibility of html, languages, title and
content.

Second note: We still need AV translations too. See
Bugzilla.

Test plan:
[1] Add a NEWSLETTER value under AV category PATRON_CONSENT.
[2] Add no OPAC description. Save. Hit OPAC consents page.
[3] Add description. Check page again.
[4] Add html to description. Check again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2022-10-03 07:04:29 UTC
(In reply to David Nind from comment #29)
> The patch no longer applies 8-(..
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 31503: Adjust db column patron_consent.type
> Applying: Bug 31503: Add $patron->consent
> Using index info to reconstruct a base tree...
> M	Koha/Patron.pm
> M	t/db_dependent/Koha/Patron.t
> Falling back to patching base and 3-way merge...
> Auto-merging t/db_dependent/Koha/Patron.t
> CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patron.t
> Auto-merging Koha/Patron.pm
> error: Failed to merge in the changes.
> Patch failed at 0001 Bug 31503: Add $patron->consent

Try again please David.
Note that this test now fails on master in subtest 19 -  notify_library_of_registration() with and without this patch set. So can be ignored, should be addressed somewhere else.
Comment 38 David Nind 2022-10-03 14:14:39 UTC
Created attachment 141263 [details] [review]
Bug 31503: Adjust db column patron_consent.type

We want to control this field now via authorised values. An ENUM
is too restrictive.
A foreign key on the values of authorised_value for category
'PATRON_CONSENT' is not possible.
This will do for now.

Test plan:
Run dbrev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2022-10-03 14:14:45 UTC
Created attachment 141264 [details] [review]
Bug 31503: Add $patron->consent

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2022-10-03 14:14:51 UTC
Created attachment 141265 [details] [review]
Bug 31503: Use patron->consent in opac-patron-consent

Includes a few small additional changes in template:
[1] op == save (more generic)
[2] agreed/disagreed replaced by 1/0
[3] toggling display of consent_info paragraph

Test plan:
Enable GDPR_Policy.
Test adding/removing consent. Check what happens with
alert, consent_info paragraph and save button.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 David Nind 2022-10-03 14:14:56 UTC
Created attachment 141266 [details] [review]
Bug 31503: Change usermenu condition

Test plan:
Check if you see Consents tab depending on GDPR_Policy or having
PATRON_CONSENT authorised values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 David Nind 2022-10-03 14:15:02 UTC
Created attachment 141267 [details] [review]
Bug 31503: Add $av_cat->values

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2022-10-03 14:15:08 UTC
Created attachment 141268 [details] [review]
Bug 31503: Make opac-patron-consent more generic

We now allow multiple consent types as defined under AV
(read: authorised values) category 'PATRON_CONSENT'.
The description is used on the form too.

This is the base patch for multiple consents. We can do some more
polishing on a follow-up or another report even.
Note: the alert for GDPR was moved to the description.

Test plan:
First check the form without any PATRON_CONSENT values in AV. Toggle
the value of pref GDPR_Policy.
Now add GDPR_PROCESSING and another one (e.g. NEWSLETTER).
Refresh the OPAC form. Verify that you can toggle consent/dissent
and that everything is saved correctly. Note only a real change updates
the timestamp.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 David Nind 2022-10-03 14:15:14 UTC
Created attachment 141269 [details] [review]
Bug 31503: Configurable consent text

Although I planned to implement this via Additional
contents (AC), I am using the AV description here after
all, since AC does not yet provide enough flexibility.

The title on the consents page is constructed from the
consent type now still, and the description is from the
OPAC description in AV (PATRON_CONSENT).

Note: AV allows you to add html in the description, although
it was not meant to do so. When we switch to AC later, we
will have the possibility of html, languages, title and
content.

Second note: We still need AV translations too. See
Bugzilla.

Test plan:
[1] Add a NEWSLETTER value under AV category PATRON_CONSENT.
[2] Add no OPAC description. Save. Hit OPAC consents page.
[3] Add description. Check page again.
[4] Add html to description. Check again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2022-10-03 14:23:20 UTC
Testing notes (using KTD).

The only thing I noted was:
1. If there is no GPPR_PROCESSING authorized value for PATRON_CONSENT, then in the OPAC > your consents the text displayed is "Consent for GDPR_PROCESSING" with Yes and No options.
2. If a GPR_PROCESSING authorized value is added without any descriptions, then the text in [1] is displayed.
3. Is that text editable, or is that just a default that should be changed/customised by updating the descriptions?

My attemopt at a consolidated test plan:
1. Apply patches.
2. Update database.
3. Restart all the things (flush_memcached and restart_all).
4. Run the tests - these should pass:
   - prove t/db_dependent/Koha/Patron.t
   - prove t/db_dependent/AuthorisedValues.t
5. Enable the GDPR_Policy system preference and test (add any URL for PrivacyPolicyURL system preference, test with enforced and permissive options):
   - Go to the OPAC
   - Log in
   - You are prompted (under your consents) to "Consent for GDPR_PROCESSING"
   - Answer Yes
   - Response is "Your consent was registered on [Date/time stamp]
   - Change consent to No
   - Response is "We registered that you did not consent on [Date/time stamp]"
6. Disable the GDPR_Policy system preference - check that there is no "your consents" section link for your account (note that there is still a page that is accessible if you access http://127.0.0.1:8080/cgi-bin/koha/opac-patron-consent.pl - but that you can't do anything and the Save button is disabled).
7. Re-enable the GDPR_Policy system preference - check that there is now a "your consents" section for your account. The previous consent should be shown.
8. Check for the new authorized value category PATRON_CONSENT. Note: Without any values, "Consent for GDPR_PROCESSING" is the only option under your account > your consents in the OPAC.
9. Add GDPR_PROCESSING without a Description or Description (OPAC).
10. Refresh the OPAC form. 
11. Verify that you can toggle consent/dissent and that everything is saved correctly:
    - Note that only a real change updates the timestamp.
    - Note the text displayed for the consent [1].
12. Add a NEWSLETTER value under the authorized value category PATRON_CONSENT without any descriptions.
13. Add a Description. Save. Refresh the OPAC consents page.
14. Add a Description (OPAC). Refresh and check the OPAC consents page again.
15. Add HTML to the OPAC description. Refresh and check the OPAC consents page again. For example: This is some text for the <strong>newsletter consent</strong> to <br/> display in the OPAC.
16. Verify that you can toggle consent/dissent for the NEWSLETTER and that everything is saved correctly.

[1] Text displayed for GDPR_PROCESSING authorized value if no text enterted into Description or Description (OPAC) in the PATRON_CONSENT authorized values.

Consent for GDPR_PROCESSING 

Please read the privacy policy. [privacy policy is linked to page URL set in PrivacyPolicyURL]

In order to keep you logged in, we need your consent to process personal data as specified in the EU General Data Protection Regulation of May 25, 2018. If you would not agree, we will need to remove your account within a reasonable time.

Do you agree with our processing of your personal data as outlined in the policy?

Yes
No
Comment 46 Marcel de Rooy 2022-10-04 11:39:48 UTC
(In reply to David Nind from comment #45)

Thanks David! Great.

> The only thing I noted was:
> 1. If there is no GPPR_PROCESSING authorized value for PATRON_CONSENT, then
> in the OPAC > your consents the text displayed is "Consent for
> GDPR_PROCESSING" with Yes and No options.
> 2. If a GPR_PROCESSING authorized value is added without any descriptions,
> then the text in [1] is displayed.
> 3. Is that text editable, or is that just a default that should be
> changed/customised by updating the descriptions?

You found a small flaw! I expected AV.GetByCode to return nothing when there is no description in authorised values. But it just returns the code you were looking for.. Will fix that now.

The idea was: If there is no description, use the historic template text (not editable). Do you want to change, add something in AV. Or in Additional Contents on a follow-up report.
Comment 47 Marcel de Rooy 2022-10-04 11:50:13 UTC
Created attachment 141302 [details] [review]
Bug 31503: (follow-up) Check return of GetByCode

AV.GetByCode returns code when there is no description. So we should
check the return.

This patch also makes the use of the Do you agree-question a bit more
consistent. No need to include it in the description.

Test plan:
Check the OPAC consents page when you have an AV description for a
consent type and when you do not. For GDPR you should see the original
GDPR text when there is no description.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2022-10-04 11:51:31 UTC
(In reply to Marcel de Rooy from comment #46)
> (In reply to David Nind from comment #45)
> 
> The idea was: If there is no description, use the historic template text
> (not editable). Do you want to change, add something in AV. Or in Additional
> Contents on a follow-up report.

This only talks about GDPR.
Comment 49 David Nind 2022-10-04 12:00:49 UTC
(In reply to Marcel de Rooy from comment #46)

> The idea was: If there is no description, use the historic template text
> (not editable). Do you want to change, add something in AV. Or in Additional
> Contents on a follow-up report.

Thanks Marcel! I wasn't sure exactly what was expected.
Comment 50 Marcel de Rooy 2022-10-04 13:38:24 UTC
From IRC:

[15:34] <ashimema> 28 has all good answers.. looks like you've thought about most of it and are planning them for a continuation in the tree of bugs..
[15:34] <ashimema> so don't let me comment block anything 🙂
Comment 51 Nick Clemens 2022-10-05 13:00:42 UTC
Minor QA tool failure

Why use AV for consents? Looking ahead to bug 20380 it seems the consents will have more options - maybe we could create a 'consent_types' table and use that?

Currently too - branch limits are half implemented - I don't see the tab if my only consent is branch limited - but I can force go to the page and see the consent options - was this a plan, or a side effect of choosing AV?

The description displays in the form, but the title is still the AV - can let go in light of 31570, but it is lees friendly to see "Consent for GDPR_PROCESSING"


+use constant AV_CONSENT => 'PATRON_CONSENT';
Why not just use the string in the search? Same question for existing:
 use constant GDPR_PROCESSING => 'GDPR_PROCESSING';
Comment 52 Marcel de Rooy 2022-10-10 11:10:10 UTC
(In reply to Nick Clemens from comment #51)

Thx for QA

> Minor QA tool failure
Fixed. Renamed sub, forgot POD header. Adding follow-up.

> Why use AV for consents? Looking ahead to bug 20380 it seems the consents
> will have more options - maybe we could create a 'consent_types' table and
> use that?
Could be done later indeed. Decided here to start with AV and its existing functionality.

> Currently too - branch limits are half implemented - I don't see the tab if
> my only consent is branch limited - but I can force go to the page and see
> the consent options - was this a plan, or a side effect of choosing AV?
Good catch. This is the most serious issue imo. And a blocker.
Would addressing this be sufficient for you to pass QA? If not, I will probably have to switch first to Additional contents fixing. And try this again for 23.05 or 23.11.

> The description displays in the form, but the title is still the AV - can
> let go in light of 31570, but it is lees friendly to see "Consent for
> GDPR_PROCESSING"
Correct. Should be addressed later on.

> +use constant AV_CONSENT => 'PATRON_CONSENT';
> Why not just use the string in the search? Same question for existing:
>  use constant GDPR_PROCESSING => 'GDPR_PROCESSING';
Using constants is good practice. When we add another instance, we can refer to the constant.
Comment 53 Marcel de Rooy 2022-10-10 11:10:52 UTC
Created attachment 141570 [details] [review]
Bug 31503: (QA follow-up) Fix POD warning

FAIL   pod coverage
    POD is missing for 'values'

I renamed the sub in the process :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Martin Renvoize 2022-10-10 15:52:58 UTC
I asked myself the same question regards the use of AV here.. and would likely ask it again if it was switch to additional_contents.. it feels like this is a big enough feature tree that starting with a fresh new table instead of trying to plumb extra functionality into other tables is the right way to go... 

The account types stuff is a clear example of where/why this makes sense.. we added lots of functionality to it really quite quickly once we'd converted from AV to distinct tables.. AV's and Additional Contents end up very restrictive if you are actually trying to tie functionality to this stuff...

You talk about future enhancements Marcel.. any change you can write up a tree of bugs to describe where your headed with the big picture?
Comment 55 Marcel de Rooy 2022-10-11 11:06:50 UTC
(In reply to Martin Renvoize from comment #54)
> I asked myself the same question regards the use of AV here.. and would
> likely ask it again if it was switch to additional_contents.. it feels like
> this is a big enough feature tree that starting with a fresh new table
> instead of trying to plumb extra functionality into other tables is the
> right way to go... 
> 
> The account types stuff is a clear example of where/why this makes sense..
> we added lots of functionality to it really quite quickly once we'd
> converted from AV to distinct tables.. AV's and Additional Contents end up
> very restrictive if you are actually trying to tie functionality to this
> stuff...
> 
> You talk about future enhancements Marcel.. any change you can write up a
> tree of bugs to describe where your headed with the big picture?

Thanks, Martin.
Reworking this along some other lines will certainly need more time.
I was certainly not planning to make this as large as Accounts ;) Baby steps..
Will come back here later.