Bug 20819 - GDPR: Add a consent field for processing personal data in account menu and self-registration
Summary: GDPR: Add a consent field for processing personal data in account menu and se...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords: release-notes-needed
Depends on: 21374
Blocks: 18081 21336 21368 21369
  Show dependency treegraph
 
Reported: 2018-05-25 10:00 UTC by Marcel de Rooy
Modified: 2018-11-12 07:54 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This report adds a new table patron_consent in order to save user consent for processing personal data (GDPR), but allows for future extension. It adds two preferences: GDPR_Policy and PrivacyPolicyURL. The first pref allows you to enforce giving consent before using the OPAC as a specific user. In permissive mode, we only show a warning on the consent page. The second pref allows you to add a URL to a privacy policy page. On the self registration page we also add asking for consent if the pref is enabled.


Attachments
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.06 KB, patch)
2018-06-04 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (5.18 KB, patch)
2018-06-04 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.19 KB, patch)
2018-06-04 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.63 KB, patch)
2018-06-05 14:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (1.92 KB, patch)
2018-06-05 14:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.18 KB, patch)
2018-06-05 14:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (4.71 KB, patch)
2018-07-09 10:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.25 KB, patch)
2018-07-09 10:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.63 KB, patch)
2018-07-09 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (1.91 KB, patch)
2018-07-09 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.18 KB, patch)
2018-07-09 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.06 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (4.71 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.25 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.63 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (1.91 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.45 KB, patch)
2018-07-25 14:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.14 KB, patch)
2018-09-03 11:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (4.71 KB, patch)
2018-09-03 11:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.25 KB, patch)
2018-09-03 11:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.63 KB, patch)
2018-09-03 11:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (1.91 KB, patch)
2018-09-03 11:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.45 KB, patch)
2018-09-03 11:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.25 KB, patch)
2018-09-14 22:24 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (4.82 KB, patch)
2018-09-14 22:24 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.36 KB, patch)
2018-09-14 22:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.74 KB, patch)
2018-09-14 22:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (2.01 KB, patch)
2018-09-14 22:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.62 KB, patch)
2018-09-14 22:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (2.01 KB, patch)
2018-09-18 20:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.31 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (4.87 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.41 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.79 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (2.07 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.76 KB, patch)
2018-09-18 20:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20819: Dbrev for new table patron_consent and sysprefs (7.35 KB, patch)
2018-09-20 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: DBIx schema changes (5.38 KB, patch)
2018-09-20 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add Koha object classes for patron consents (5.45 KB, patch)
2018-09-20 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add your consents tab to opac-user (9.83 KB, patch)
2018-09-20 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add check in get_template_and_user to enforce GDPR (2.11 KB, patch)
2018-09-20 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.65 KB, patch)
2018-09-20 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: Add consent to self-registration process (5.74 KB, patch)
2018-09-20 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20819: (RM follow-up) Add missing filters (4.43 KB, patch)
2018-09-20 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20819: (Follow-up) Show more clearly that a user gave no consent (1.78 KB, patch)
2018-09-21 06:05 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 2018-05-25 10:00:44 UTC
See 18081
Comment 1 Marcel de Rooy 2018-06-04 12:47:24 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2018-06-04 12:47:28 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2018-06-04 12:47:32 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2018-06-04 12:48:39 UTC
More patches forthcoming tomorrow or so.
Comment 5 Marcel de Rooy 2018-06-05 14:08:09 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2018-06-05 14:08:13 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2018-06-05 14:08:18 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2018-07-09 10:00:54 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2018-07-09 10:00:58 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2018-07-09 10:01:03 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2018-07-09 10:01:08 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2018-07-09 10:01:12 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2018-07-09 10:02:32 UTC
Trivial rebase
Comment 14 Jon Knight 2018-07-09 12:56:43 UTC
Do we need to keep a copy of the template in use during the consent in the database along with the borrower ID and date?  The UK ICO page on consenting seems to say we do:

https://ico.org.uk/for-organisations/guide-to-the-general-data-protection-regulation-gdpr/consent/how-should-we-obtain-record-and-manage-consent/

(second example).
Comment 15 Marcel de Rooy 2018-07-13 10:27:03 UTC
(In reply to Jon Knight from comment #14)
> Do we need to keep a copy of the template in use during the consent in the
> database along with the borrower ID and date?  The UK ICO page on consenting
> seems to say we do:
> 
> https://ico.org.uk/for-organisations/guide-to-the-general-data-protection-
> regulation-gdpr/consent/how-should-we-obtain-record-and-manage-consent/
> 
> (second example).

Thx for your interest in this patch.

The link says:
===
If consent was given online, your records should include the data submitted as well as a timestamp to link it to the relevant version of the data capture form.
You keep records that include an ID and the data submitted online together with a timestamp. You also keep a copy of the version of the data-capture form and any other relevant documents in use at that date.
===

This patch registers the borrower ID and the timestamp of consent given. What each library should keep somehow (not described), is: a version history of the template, esp. the exact text for the consent and a history of the referenced privacy page text. In that way you can 'prove' that the user gave consent for a specific version of template and privacy page.
(Note: the privacy page is only specified in this patch by a preference called PrivacyPolicyURL. The library itself should create that local page.)

I am no legal expert, but I can't imagine that we need to save these texts at an individual consent level.
Hope this is clear enough?
Comment 16 Jon Knight 2018-07-16 09:02:12 UTC
I was focusing on the example box with the tick (in other words the right, approved way the ICO expect this to be handled) that says:

"You keep records that include an ID and the data submitted online together with a timestamp. You also keep a copy of the version of the data-capture form and any other relevant documents in use at that date."

So to me that does imply the need to save these texts at an individual consent level, as they all form part of the consent package for that user.

Indeed the "crossed" part (wrong) of that example says, 

"You keep the time and date of consent linked to an IP address, with a web link to your current data-capture form and privacy policy" 

so they seem to think that just knowing what the PrivacyPolicyURL was set to at the time of consent is not enough.
Comment 17 Marcel de Rooy 2018-07-16 09:32:50 UTC
I was looking at the same text as you. But I concluded that we need to save the consent tekst somewhere in a version history not on consent level (that would be unneeded redundancy).
But not implying that saving a URL would be enough.
Comment 18 Marcel de Rooy 2018-07-25 14:35:02 UTC
Probably changes on Member..
Comment 19 Marcel de Rooy 2018-07-25 14:51:23 UTC
Created attachment 77264 [details] [review]
Bug 20819: Dbrev for new table patron_consent and sysprefs

[1] We add table patron_consent.
It allows for future extension. We could e.g. save consent for newsletter etc.
[2] Add one field to borrower_modifications
Datetime the patron who self-registered, gave consent.
[3] Syspref PrivacyPolicyURL
[4] Syspref GDPR_Policy: enforced, permissive or disabled.

Test plan:
[1] Run the dbrev or do a new install.
[2] Look at the description of the new prefs in Administration.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2018-07-25 14:51:28 UTC
Created attachment 77265 [details] [review]
Bug 20819: DBIx schema changes

No test plan.
Comment 21 Marcel de Rooy 2018-07-25 14:51:33 UTC
Created attachment 77266 [details] [review]
Bug 20819: Add Koha object classes for patron consents

Introduces Koha::Patron::Consent[s] for new table.
Adds basic CRUD test.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2018-07-25 14:51:38 UTC
Created attachment 77267 [details] [review]
Bug 20819: Add your consents tab to opac-user

This patch adds a consents tab to the OPAC user account menu. We now
add a GDPR section here, but it is open for future extensions. Think of
a newsletter checkbox for instance.

Script opac-patron-consent handles the tab. And now only includes some
GDPR code but is also written for more general use too.

Test plan:
[1] Set GDPR_Policy pref to Disabled. Verify that OPAC operates as usual.
[2] Set pref to Permissive. Try to save a consent or a refusal. Note that
    you are not logged out when saving a refusal.
[3] Set pref to Enforced. Save a refusal. You should be logged out.
    Log in again and verify that the consents tab shows a No.
    Note: a follow-up patch will add further enforcements.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2018-07-25 14:51:42 UTC
Created attachment 77268 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2018-07-25 14:51:47 UTC
Created attachment 77269 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2018-07-25 14:52:45 UTC
Rebased.
Especially last patch had conflicts due to AddMember changes.
Comment 26 Marcel de Rooy 2018-09-03 11:28:40 UTC
Created attachment 78369 [details] [review]
Bug 20819: Dbrev for new table patron_consent and sysprefs

[1] We add table patron_consent.
It allows for future extension. We could e.g. save consent for newsletter etc.
[2] Add one field to borrower_modifications
Datetime the patron who self-registered, gave consent.
[3] Syspref PrivacyPolicyURL
[4] Syspref GDPR_Policy: enforced, permissive or disabled.

Test plan:
[1] Run the dbrev or do a new install.
[2] Look at the description of the new prefs in Administration.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2018-09-03 11:28:45 UTC
Created attachment 78370 [details] [review]
Bug 20819: DBIx schema changes

No test plan.
Comment 28 Marcel de Rooy 2018-09-03 11:28:50 UTC
Created attachment 78371 [details] [review]
Bug 20819: Add Koha object classes for patron consents

Introduces Koha::Patron::Consent[s] for new table.
Adds basic CRUD test.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2018-09-03 11:28:56 UTC
Created attachment 78372 [details] [review]
Bug 20819: Add your consents tab to opac-user

This patch adds a consents tab to the OPAC user account menu. We now
add a GDPR section here, but it is open for future extensions. Think of
a newsletter checkbox for instance.

Script opac-patron-consent handles the tab. And now only includes some
GDPR code but is also written for more general use too.

Test plan:
[1] Set GDPR_Policy pref to Disabled. Verify that OPAC operates as usual.
[2] Set pref to Permissive. Try to save a consent or a refusal. Note that
    you are not logged out when saving a refusal.
[3] Set pref to Enforced. Save a refusal. You should be logged out.
    Log in again and verify that the consents tab shows a No.
    Note: a follow-up patch will add further enforcements.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2018-09-03 11:29:01 UTC
Created attachment 78373 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2018-09-03 11:29:06 UTC
Created attachment 78374 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2018-09-03 11:30:29 UTC
Needed a rebase for removed NorwegianDB prefs.
Comment 33 sandboxes@biblibre.com 2018-09-14 22:23:16 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 34 sandboxes@biblibre.com 2018-09-14 22:24:54 UTC
Created attachment 78779 [details] [review]
Bug 20819: Dbrev for new table patron_consent and sysprefs

[1] We add table patron_consent.
It allows for future extension. We could e.g. save consent for newsletter etc.
[2] Add one field to borrower_modifications
Datetime the patron who self-registered, gave consent.
[3] Syspref PrivacyPolicyURL
[4] Syspref GDPR_Policy: enforced, permissive or disabled.

Test plan:
[1] Run the dbrev or do a new install.
[2] Look at the description of the new prefs in Administration.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 35 sandboxes@biblibre.com 2018-09-14 22:24:57 UTC
Created attachment 78781 [details] [review]
Bug 20819: DBIx schema changes

No test plan.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 36 sandboxes@biblibre.com 2018-09-14 22:25:01 UTC
Created attachment 78783 [details] [review]
Bug 20819: Add Koha object classes for patron consents

Introduces Koha::Patron::Consent[s] for new table.
Adds basic CRUD test.

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

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 37 sandboxes@biblibre.com 2018-09-14 22:25:09 UTC
Created attachment 78784 [details] [review]
Bug 20819: Add your consents tab to opac-user

This patch adds a consents tab to the OPAC user account menu. We now
add a GDPR section here, but it is open for future extensions. Think of
a newsletter checkbox for instance.

Script opac-patron-consent handles the tab. And now only includes some
GDPR code but is also written for more general use too.

Test plan:
[1] Set GDPR_Policy pref to Disabled. Verify that OPAC operates as usual.
[2] Set pref to Permissive. Try to save a consent or a refusal. Note that
    you are not logged out when saving a refusal.
[3] Set pref to Enforced. Save a refusal. You should be logged out.
    Log in again and verify that the consents tab shows a No.
    Note: a follow-up patch will add further enforcements.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 38 sandboxes@biblibre.com 2018-09-14 22:25:14 UTC
Created attachment 78786 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 39 sandboxes@biblibre.com 2018-09-14 22:25:19 UTC
Created attachment 78788 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 40 Marcel de Rooy 2018-09-17 07:40:04 UTC
(In reply to sandboxes@biblibre.com from comment #39)
> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Thanks Séverine!
Comment 41 Josef Moravec 2018-09-18 20:18:49 UTC
Created attachment 79059 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 42 Josef Moravec 2018-09-18 20:34:16 UTC
Created attachment 79062 [details] [review]
Bug 20819: Dbrev for new table patron_consent and sysprefs

[1] We add table patron_consent.
It allows for future extension. We could e.g. save consent for newsletter etc.
[2] Add one field to borrower_modifications
Datetime the patron who self-registered, gave consent.
[3] Syspref PrivacyPolicyURL
[4] Syspref GDPR_Policy: enforced, permissive or disabled.

Test plan:
[1] Run the dbrev or do a new install.
[2] Look at the description of the new prefs in Administration.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 43 Josef Moravec 2018-09-18 20:34:23 UTC
Created attachment 79063 [details] [review]
Bug 20819: DBIx schema changes

No test plan.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 44 Josef Moravec 2018-09-18 20:34:32 UTC
Created attachment 79064 [details] [review]
Bug 20819: Add Koha object classes for patron consents

Introduces Koha::Patron::Consent[s] for new table.
Adds basic CRUD test.

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

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2018-09-18 20:34:38 UTC
Created attachment 79065 [details] [review]
Bug 20819: Add your consents tab to opac-user

This patch adds a consents tab to the OPAC user account menu. We now
add a GDPR section here, but it is open for future extensions. Think of
a newsletter checkbox for instance.

Script opac-patron-consent handles the tab. And now only includes some
GDPR code but is also written for more general use too.

Test plan:
[1] Set GDPR_Policy pref to Disabled. Verify that OPAC operates as usual.
[2] Set pref to Permissive. Try to save a consent or a refusal. Note that
    you are not logged out when saving a refusal.
[3] Set pref to Enforced. Save a refusal. You should be logged out.
    Log in again and verify that the consents tab shows a No.
    Note: a follow-up patch will add further enforcements.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2018-09-18 20:34:44 UTC
Created attachment 79066 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2018-09-18 20:34:49 UTC
Created attachment 79067 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Marcel de Rooy 2018-09-19 11:10:34 UTC
(In reply to Josef Moravec from comment #47)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Thanks Josef
Comment 49 Marcel de Rooy 2018-09-20 08:25:44 UTC
Created attachment 79129 [details] [review]
Bug 20819: Dbrev for new table patron_consent and sysprefs

[1] We add table patron_consent.
It allows for future extension. We could e.g. save consent for newsletter etc.
[2] Add one field to borrower_modifications
Datetime the patron who self-registered, gave consent.
[3] Syspref PrivacyPolicyURL
[4] Syspref GDPR_Policy: enforced, permissive or disabled.

Test plan:
[1] Run the dbrev or do a new install.
[2] Look at the description of the new prefs in Administration.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Marcel de Rooy 2018-09-20 08:25:49 UTC
Created attachment 79130 [details] [review]
Bug 20819: DBIx schema changes

No test plan.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Marcel de Rooy 2018-09-20 08:25:55 UTC
Created attachment 79131 [details] [review]
Bug 20819: Add Koha object classes for patron consents

Introduces Koha::Patron::Consent[s] for new table.
Adds basic CRUD test.

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

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Marcel de Rooy 2018-09-20 08:26:00 UTC
Created attachment 79132 [details] [review]
Bug 20819: Add your consents tab to opac-user

This patch adds a consents tab to the OPAC user account menu. We now
add a GDPR section here, but it is open for future extensions. Think of
a newsletter checkbox for instance.

Script opac-patron-consent handles the tab. And now only includes some
GDPR code but is also written for more general use too.

Test plan:
[1] Set GDPR_Policy pref to Disabled. Verify that OPAC operates as usual.
[2] Set pref to Permissive. Try to save a consent or a refusal. Note that
    you are not logged out when saving a refusal.
[3] Set pref to Enforced. Save a refusal. You should be logged out.
    Log in again and verify that the consents tab shows a No.
    Note: a follow-up patch will add further enforcements.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Marcel de Rooy 2018-09-20 08:26:05 UTC
Created attachment 79133 [details] [review]
Bug 20819: Add check in get_template_and_user to enforce GDPR

If you choose to enforce GDPR policy, a user needs to give consent for
data processing before he does something else in the OPAC while being
logged in.

Test plan:
[1] Set GDPR_Policy to Disabled or Permissive. Usual behavior.
[2] Set to Enforced. Save a refusal on your consents. Notice that
    you are logged out when saving. When you login again, all OPAC
    requests are redirected to your consents tab.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 54 Marcel de Rooy 2018-09-20 08:26:10 UTC
Created attachment 79134 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 55 Marcel de Rooy 2018-09-20 08:28:19 UTC
Resolved a checksum problem in patch 2 and a git problem for patch 6.
Comment 56 Marcel de Rooy 2018-09-20 08:32:04 UTC
Created attachment 79135 [details] [review]
Bug 20819: Add consent to self-registration process

We add a section for the GDPR consent in opac-memberentry (only for the
self-registration). Not when editing personal details.

Test plan:
[1] Enable selfregistration (with confirm) and GDPR policy.
[2] Register a new account in OPAC. Verify that the GDPR checkbox is
    required.
[3] After you submit, you should see a date in borrower_modifications
    field gdpr_proc_consent.
[4] When you confirm, verify that the consent is visible on your consents.
[5] Enable selfregistration without confirmation mail. Register again.
[6] Check your consents tab again.

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

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Marcel de Rooy 2018-09-20 08:32:46 UTC
(In reply to Marcel de Rooy from comment #55)
> Resolved a checksum problem in patch 2 and a git problem for patch 6.

Patch 6 had no problem; forgot to apply dependency on 21374
Comment 58 Nick Clemens 2018-09-20 13:41:58 UTC
Created attachment 79175 [details] [review]
Bug 20819: (RM follow-up) Add missing filters
Comment 59 Nick Clemens 2018-09-20 13:47:28 UTC
Awesome work all!

Pushed to master for 18.11
Comment 60 Nick Clemens 2018-09-20 13:49:11 UTC
I pushed this, the UI display for when a user has denied consent could be improved - it still seems to be waiting for approval, but doesn't make it clear that 'No' has been set - as others seemed fine with it I let it go but something to consider
Comment 61 Marcel de Rooy 2018-09-21 06:01:05 UTC
(In reply to Nick Clemens from comment #58)
> Created attachment 79175 [details] [review] [review]
> Bug 20819: (RM follow-up) Add missing filters

Thx
Comment 62 Marcel de Rooy 2018-09-21 06:05:46 UTC
Created attachment 79190 [details] [review]
Bug 20819: (Follow-up) Show more clearly that a user gave no consent

See comment60 on Bugzilla.
Although I intentionally did not want to show date and time of refusal,
we can obviously add one line saying that we already have a no.

Test plan:
Login without consent and see no additional line.
Agree and verify that you see the registered on xxx line.
Disagree and verify that you now see "You indicated recently...".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2018-09-21 06:06:46 UTC
(In reply to Nick Clemens from comment #60)
> I pushed this, the UI display for when a user has denied consent could be
> improved - it still seems to be waiting for approval, but doesn't make it
> clear that 'No' has been set - as others seemed fine with it I let it go but
> something to consider

See attachment. Your consent please :)
Comment 64 Nick Clemens 2018-09-21 13:50:05 UTC
(In reply to Marcel de Rooy from comment #62)
> Created attachment 79190 [details] [review] [review]
> Bug 20819: (Follow-up) Show more clearly that a user gave no consent
> 
> See comment60 on Bugzilla.
> Although I intentionally did not want to show date and time of refusal,
> we can obviously add one line saying that we already have a no.
> 
> Test plan:
> Login without consent and see no additional line.
> Agree and verify that you see the registered on xxx line.
> Disagree and verify that you now see "You indicated recently...".
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Followup pushed to master for 18.11, thank you!
Comment 65 Martin Renvoize 2018-09-24 12:40:26 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 66 Jonathan Druart 2018-11-02 13:32:09 UTC
+        my $consent = Koha::Patron::Consents->search({
+            borrowernumber => getborrowernumber($user),
+            type => 'GDPR_PROCESSING',
+            given_on => { '!=', undef },
+        })->next;
+        if( !$consent ) {


->count could have been better.
Comment 67 Marcel de Rooy 2018-11-07 15:34:52 UTC
(In reply to Jonathan Druart from comment #66)
> +        my $consent = Koha::Patron::Consents->search({
> +            borrowernumber => getborrowernumber($user),
> +            type => 'GDPR_PROCESSING',
> +            given_on => { '!=', undef },
> +        })->next;
> +        if( !$consent ) {
> 
> 
> ->count could have been better.

I tested that assumption a bit. And it seems not to be true in terms of performance at least?
In short:
1.3480 sec for 1000 counts and 0.0094 for 1000 next's (on 1000 records).
It seems that DBIx goes to the db for each count but the repeated nexts are handled much more efficiently (cache, prefetch, ..)?

Script fragment on next comment.
Comment 68 Marcel de Rooy 2018-11-07 15:35:11 UTC
    my $set = Koha::Patron::Consents->search({ borrowernumber => $firstid });
    #my $set = Koha::Patron::Consents->search;
    my $t1 = time;
    foreach(1..100) { my $i = $set->count; }
    my $t2 = time;
    is(1, 1, sprintf( "COUNT %6.4f\n", $t2 - $t1 ) );
    $t1 = time;
    foreach(1..100) { my $i = $set->next; }
    #foreach(1..1000) { my $i = $set->next; #$set->_resultset->first unless $i; }
    $t2 = time;
    is(1, 1, sprintf( "NEXT %6.4f\n", $t2 - $t1 ) );
Comment 69 Marcel de Rooy 2018-11-07 15:53:10 UTC
use Modern::Perl;
use Data::Dumper qw/Dumper/;
use Test::More tests => 1;
use Time::HiRes qw/time/;

use t::lib::Mocks;
use t::lib::TestBuilder;

use Koha::Patron::Consents;

my $schema = Koha::Database->new->schema;
my $builder = t::lib::TestBuilder->new;

$schema->storage->txn_begin;
subtest 'myTest' => sub {
    plan tests => 2;

    # Add 1000 consents
    my $firstid;
    foreach(1..100) {
        my $consent = $builder->build_object({ class => 'Koha::Patron::Consents' });
        $firstid = $consent->borrowernumber if !$firstid;
    }
    my $set = Koha::Patron::Consents->search({ borrowernumber => $firstid });
    #my $set = Koha::Patron::Consents->search;
    my $t1 = time;
    foreach(1..100) { my $i = $set->count; }
    my $t2 = time;
    is(1, 1, sprintf( "COUNT %6.4f\n", $t2 - $t1 ) );
    $t1 = time;
    foreach(1..100) { my $i = $set->next; }
    #foreach(1..1000) { my $i = $set->next; #$set->_resultset->first unless $i; }
    $t2 = time;
    is(1, 1, sprintf( "NEXT %6.4f\n", $t2 - $t1 ) );
};
$schema->storage->txn_rollback;
Comment 70 Jonathan Druart 2018-11-07 16:01:13 UTC
Marcel, use DBIC_TRACE=1 to understand what's going on.

You are comparing 100 queries vs 1 query: 100 "SELECT COUNT" vs 1 "SELECT *"

The ->next will return something only on first pass.
Comment 71 Martin Renvoize 2018-11-08 07:52:55 UTC
Agreed, that test isn't testing what you think Marcel.

The ->next is chained onto the resultset inside the loop in C4::Auth whereas the resultset is created outside of the loop in your test.

I actually don't know which is faster, but I would imagine count is a bit faster and more memory efficient.

Try moving the queries inside the loop and test again, I'd certainly be interested in the results.

(a correlate subquery version might be interesting too)
Comment 72 Martin Renvoize 2018-11-08 07:54:54 UTC
Ignore the subquery bit.. that doesn't apply here (though I'd still be interested in comparing that at some point)
Comment 73 Marcel de Rooy 2018-11-08 14:20:38 UTC
(In reply to Jonathan Druart from comment #70)
> Marcel, use DBIC_TRACE=1 to understand what's going on.
> 
> You are comparing 100 queries vs 1 query: 100 "SELECT COUNT" vs 1 "SELECT *"
> 
> The ->next will return something only on first pass.

Yeah I have been toggling a bit with comment chars. But I also tested the next on the set of 100 records.
Comment 74 Marcel de Rooy 2018-11-08 14:25:03 UTC
# Subtest: myTest
    1..2
    ok 1 - COUNT 0.1278
    #
    ok 2 - NEXT 0.0106
    #
18021068316832683368346835683668376838683968406841684268436844684568466847684868496850685168526853685468556856685768586859686068616862686368646865686668676868686968706871687268736874687568766877687868796880688168826883688468856886688768886889689068916892689368946895689668976898689969006901690269036904690569066907690869096910691169126913691469156916691769186919692069216922692369246925692669276928

with a slight variation
Comment 75 Marcel de Rooy 2018-11-08 14:25:34 UTC
use Modern::Perl;
use Data::Dumper qw/Dumper/;
use Test::More tests => 1;
use Time::HiRes qw/time/;

use t::lib::Mocks;
use t::lib::TestBuilder;

use Koha::Patron::Consents;

my $schema = Koha::Database->new->schema;
my $builder = t::lib::TestBuilder->new;

$schema->storage->txn_begin;
subtest 'myTest' => sub {
    plan tests => 2;

    # Add 1000 consents
    my $firstid;
    foreach(1..100) {
        my $consent = $builder->build_object({ class => 'Koha::Patron::Consents' });
        $firstid = $consent->borrowernumber if !$firstid;
    }
    #my $set = Koha::Patron::Consents->search({ borrowernumber => $firstid });
    my $set = Koha::Patron::Consents->search;
    my $t1 = time;
    foreach(1..100) { my $i = $set->count; }
    my $t2 = time;
    is(1, 1, sprintf( "COUNT %6.4f\n", $t2 - $t1 ) );
    my $str = '';
    $t1 = time;
    foreach(1..100) { my $i = $set->next; $str.= $i->id; }
    #foreach(1..1000) { my $i = $set->next; #$set->_resultset->first unless $i; }
    $t2 = time;
    is(1, 1, sprintf( "NEXT %6.4f\n", $t2 - $t1 ) );
    print "$str\n";
};
$schema->storage->txn_rollback;
Comment 76 Marcel de Rooy 2018-11-08 14:26:49 UTC
(In reply to Martin Renvoize from comment #71)
> The ->next is chained onto the resultset inside the loop in C4::Auth whereas
> the resultset is created outside of the loop in your test.
Yeah deliberately.

> I actually don't know which is faster, but I would imagine count is a bit
> faster and more memory efficient.
Thats the problem. I would expect something else.
Comment 77 Marcel de Rooy 2018-11-08 14:36:03 UTC
On 1000 records:
    ok 1 - COUNT 1.3365
    ok 2 - NEXT 0.0950
The printed string shows all 1000 primary keys too. What am I missing here?
Comment 78 Martin Renvoize 2018-11-08 14:44:38 UTC
(In reply to Marcel de Rooy from comment #76)
> (In reply to Martin Renvoize from comment #71)
> > The ->next is chained onto the resultset inside the loop in C4::Auth whereas
> > the resultset is created outside of the loop in your test.
> Yeah deliberately.

OK, I'm confused then.. what are you trying to prove here then?

> 
> > I actually don't know which is faster, but I would imagine count is a bit
> > faster and more memory efficient.
> Thats the problem. I would expect something else.

When doing it inside the loop as per my suggestion what you're actually doing is 

SELECT COUNT(*) FROM whatever WHERE whatever;  Cache this answer and print it 100 times

vs

SELECT * FROM whatever WHERE whatever; Do this 100 times, hitting the database for each call of 'Next' to get the next result given an offset from the database.  (Unless you call prefetch which says 'get me all rows in one go and cache them to iterate in memory)
Comment 79 Marcel de Rooy 2018-11-12 07:54:21 UTC
From IRC:
See https://metacpan.org/source/RI[…]esultSet.pm#L1594
count is cached at the dbic level here
which is why recursing count on the same resultset should be very very fast.
https://metacpan.org/source/RI[…]esultSet.pm#L1229
next gets really clever ;)
there's loads of optimizatino in there regarding caching rows
see _construct_results


It seems that count is not cached in my test (so recursing not fast) and next is cached. I am not triggered right now to replace the next in Auth.pm. And for one next we are talking about milliseconds.