Bug 23150 - GDPR feature breaks patron self modification on OPAC
Summary: GDPR feature breaks patron self modification on OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Nick Clemens
URL:
Keywords:
Depends on: 20819
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-18 18:34 UTC by Andrew Fuerste-Henry
Modified: 2020-11-30 21:45 UTC (History)
5 users (show)

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


Attachments
Bug 23150: Do not consider gdpr_proc_consent a mandatory field (1.30 KB, patch)
2019-06-19 19:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23150: Do not consider gdpr_proc_consent a mandatory field (1.40 KB, patch)
2019-06-20 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23150: (follow-up) Make gdpr_proc_consent mandatory for create (1.03 KB, patch)
2019-06-20 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23150: Do not consider gdpr_proc_consent a mandatory field (1.41 KB, patch)
2019-06-21 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23150: (follow-up) Make gdpr_proc_consent mandatory for create (1.05 KB, patch)
2019-06-21 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2019-06-18 18:34:37 UTC
To Test:
- Set GDPR_Policy to either Enforced or Permissive
- Set PatronSelfRegistrationBorrowerMandatoryField to empty
- Set OPACPatronDetails to Allow
- Log into the OPAC
- Go to opac-memberentry.pl, change something, submit modification request
- Request is refused with message "You have not filled out all required fields. Please fill in all missing fields and resubmit."
Comment 1 Jonathan Druart 2019-06-19 19:53:09 UTC
Created attachment 90817 [details] [review]
Bug 23150: Do not consider gdpr_proc_consent a mandatory field

It introduced a regression in the patron's self modification feature.
Patron's details are no longer modificable, the screen displays
"You have not filled out all required fields. Please fill in all missing
fields and resubmit."

Test plan:
- Set GDPR_Policy to either Enforced or Permissive
- Set PatronSelfRegistrationBorrowerMandatoryField to empty
- Set OPACPatronDetails to Allow
- Log into the OPAC
- Accept the GDPR screen
- Go to opac-memberentry.pl, change something, submit modification
request
Comment 2 Jonathan Druart 2019-06-19 19:53:54 UTC
Not sure about this fix, I'd like Marcel review :)
Comment 3 Marcel de Rooy 2019-06-20 06:42:56 UTC
(In reply to Andrew from comment #0)
> - Request is refused with message "You have not filled out all required
> fields. Please fill in all missing fields and resubmit."

I am not sure if I understand. You only need to tick the GDPR consent field, and you can save the record?
Note that if you do not agree with the GDPR policy, we do not want you to proceed and create an account.

So the title GDPR breaks self registration, is completely unclear to me.
Comment 4 Marcel de Rooy 2019-06-20 06:44:49 UTC
(In reply to Jonathan Druart from comment #2)
> Not sure about this fix, I'd like Marcel review :)

At least for mode Enforced, we should treat it as mandatory. But I think for Permissive it should better be handled as mandatory too. But that is more arbitrary.
This change actually breaks GDPR ;)
Comment 5 Andrew Fuerste-Henry 2019-06-20 12:15:45 UTC
This is actually an issue on patron self *modification* rather than registration.

If GDPR_policy is set to enforced or permissive, patron self modification will always give a "missing required field" error, even if the patron has already accepted the GDPR terms. There is nothing GDPR-related on the self-modification page, so there is nothing in the patron can do to move past this error and nothing indicating what the error is referring to.
Comment 6 Marcel de Rooy 2019-06-20 12:33:14 UTC
(In reply to Andrew from comment #5)
> This is actually an issue on patron self *modification* rather than
> registration.
> 
> If GDPR_policy is set to enforced or permissive, patron self modification
> will always give a "missing required field" error, even if the patron has
> already accepted the GDPR terms. There is nothing GDPR-related on the
> self-modification page, so there is nothing in the patron can do to move
> past this error and nothing indicating what the error is referring to.

Ahh. Bad reader ;)
Probably this fix then still needs a little tweak. Looking now.
Comment 7 Andrew Fuerste-Henry 2019-06-20 12:38:29 UTC
I sort of anticipated confusion because I pointed at opac-memberentry.pl, which is confusingly both self reg and self mod :P
Comment 8 Marcel de Rooy 2019-06-20 12:41:45 UTC
Created attachment 90846 [details] [review]
Bug 23150: Do not consider gdpr_proc_consent a mandatory field

It introduced a regression in the patron's self modification feature.
Patron's details are no longer modificable, the screen displays
"You have not filled out all required fields. Please fill in all missing
fields and resubmit."

Test plan:
- Set GDPR_Policy to either Enforced or Permissive
- Set PatronSelfRegistrationBorrowerMandatoryField to empty
- Set OPACPatronDetails to Allow
- Log into the OPAC
- Accept the GDPR screen
- Go to opac-memberentry.pl, change something, submit modification
request

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2019-06-20 12:41:49 UTC
Created attachment 90847 [details] [review]
Bug 23150: (follow-up) Make gdpr_proc_consent mandatory for create

When updating (patron modification), the field should not be required.

Test plan:
Check patron modification on opac.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2019-06-20 12:42:10 UTC
(In reply to Andrew from comment #7)
> I sort of anticipated confusion because I pointed at opac-memberentry.pl,
> which is confusingly both self reg and self mod :P

Fixed
Comment 11 Katrin Fischer 2019-06-20 12:47:40 UTC
GDPR is a bit painful, but still changing the title slightly :) The law is not to blame here.
Comment 12 Nick Clemens 2019-06-21 10:28:34 UTC
Created attachment 90889 [details] [review]
Bug 23150: Do not consider gdpr_proc_consent a mandatory field

It introduced a regression in the patron's self modification feature.
Patron's details are no longer modificable, the screen displays
"You have not filled out all required fields. Please fill in all missing
fields and resubmit."

Test plan:
- Set GDPR_Policy to either Enforced or Permissive
- Set PatronSelfRegistrationBorrowerMandatoryField to empty
- Set OPACPatronDetails to Allow
- Log into the OPAC
- Accept the GDPR screen
- Go to opac-memberentry.pl, change something, submit modification
request

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2019-06-21 10:28:36 UTC
Created attachment 90890 [details] [review]
Bug 23150: (follow-up) Make gdpr_proc_consent mandatory for create

When updating (patron modification), the field should not be required.

Test plan:
Check patron modification on opac.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Martin Renvoize 2019-06-21 12:14:01 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Katrin Fischer 2019-07-13 09:59:07 UTC
As the GDPR feature is in Koha down to at least 18.11... can we have this backported?
Comment 16 Fridolin Somers 2019-07-16 06:29:55 UTC
Pushed to 19.05.x for 19.05.02
Comment 17 Katrin Fischer 2019-09-24 16:52:20 UTC
I am not sure, but this appears seems to be ok in 18.11:

    my @fields = split( /\|/, $BorrowerMandatoryField );
    push @fields, 'gdpr_proc_consent' if C4::Context->preference('GDPR_Policy') && $action eq 'create';

    foreach (@fields) {
Comment 18 Katrin Fischer 2019-09-24 16:52:39 UTC
not sure = please double check :)