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>
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>
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>
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.
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
(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.
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 ;)
Having another look here
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>
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>
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>
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>
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>
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>
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>
The experimental patch with Additional Contents is obsoleted. The feature is not flexible enough yet. Will open another report to move to AC later.
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.
(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.
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
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>
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>
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>
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>
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>
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>
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>
(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.
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>
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>
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>
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>
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>
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>
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>
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
(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.
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>
(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.
(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.
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 [U+1F642]
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';
(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.
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>
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?
(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.
I will try to get this further now again with a plugin approach (with the courage of despair..) It will use plugin hook patron_consent_type.
Created attachment 155000 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155001 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155002 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155003 [details] [review] DO NOT PUSH: Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This is another approach to get this further using plugins instead of Authorised values, Additional Contents or database translations. These three areas do currently not offer enough flexibility, translation options, etc. Some developments in that area are stuck for longer time already.
David: Could you please test this change? The looks are quite the same, but the internal implementation is quite different.
Created attachment 155177 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155178 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155179 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 155180 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. Apply patches - got a weird message (see [1]). 2. Update database (updatedatabase). 3. Restart everything (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. Install and enable the plugin included with the patches: perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" 6. Restart everything (flush_memcached and restart_all) (I had to do this, otherwise I got an error trace when accessing the OPAC account consents page). 7. Enable the new system preference OPACCustomConsentTypes. 8. Go to the OPAC > [user account] > Consents tab. 9. If the PrivacyPolicyConsent system preference is disabled, then only the Newsletter consent is displayed: 9.1 If you select Yes + Save = "Your consent was registered on [date and timestamp]." 9.2 If you select No + Save = "We registered that you did not consent on [date and timestamp]." 10. The standard PrivacyPolicyConsent system preference options should continue to work as expected if OPACCustomConsentTypes is set to 'Don't allow' (enter a URL in PrivacyPolicyURL, for example http://example.com): 10.1 disabled: No consent tab in OPAC account area) 10.2 enforced: Verify that you can agree (Yes) or disagree (No) with the consent and press Save: . If Yes, a comment is added: "Your consent was registered on [date and timestamp]." . If No, you are logged out. (A comment is added under consents: "We registered that you did not consent on [date and timestamp]." - you only see this if you log in again) 10.3 permissive: Verify that you can agree (Yes) or disagree (No) with the consent and press Save: . If Yes, a comment is added: "Your consent was registered on [date and timestamp]." . If No, a comment is added and you are not logged out: "We registered that you did not consent on [date and timestamp]." 11. If both OPACCustomConsentTypes and PrivacyPolicyConsent system prferences are enabled, in addition to the default 'Privacy policy consent', there is now the Newsletter consent - the options for PrivacyPolicyConsent should continue to work as expected: 11.1 disabled: the default privacy policy consent doesn't appear, but the Newsletter consent (from the plugin) appears as per step 9: 11.2 enforced: same as for step 10.2 for the privacy policy consent, and step 9 for the newsletter consent 11.3 permissive: same as for step 10.3 for the privacy policy consent, and step 9 for the newsletter consent I hope I tested this correctly! I added a follow-up patch to amend the OPACCustomConsentTypes description - note in CAPS and bold (the same as other Notes), and linked to PrivacyPolicyConsent. I had trouble attaching the patches because of the example plugin patch - I hope I haven't screwed anything up! [1] Output I get when applying the patches: git bz apply 31503 Bug 31503 - Allow several consent types on the consents tab of OPAC account page 155000 - Bug 31503: Change patron_consent.type 155001 - Bug 31503: Change usermenu condition, add preference 155002 - Bug 31503: Make opac-patron-consent more generic 155003 - DO NOT PUSH: Add example plugin Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 31503: Change patron_consent.type Applying: Bug 31503: Change usermenu condition, add preference Applying: Bug 31503: Make opac-patron-consent more generic Applying: DO NOT PUSH: Add example plugin Moving to starting point Adding bug reference 6b2e2e2 DO NOT PUSH: Add example plugin Cleaning up back to original state on error Traceback (most recent call last): File "/usr/bin/git-bz", line 2834, in <module> applied = do_apply(bug_ref) File "/usr/bin/git-bz", line 1977, in do_apply add_url(bug, commits) File "/usr/bin/git-bz", line 1719, in add_url add_url_to_head_commit(commit, bug) File "/usr/bin/git-bz", line 1682, in add_url_to_head_commit git.commit(file="-", amend=True, _input=input) File "/usr/bin/git-bz", line 197, in f o = git_run(command, *args, **kwargs) File "/usr/bin/git-bz", line 167, in git_run output, error = process.communicate(input) File "/usr/lib/python3.9/subprocess.py", line 1134, in communicate stdout, stderr = self._communicate(input, endtime, timeout) File "/usr/lib/python3.9/subprocess.py", line 1961, in _communicate input_view = memoryview(self._input) TypeError: memoryview: a bytes-like object is required, not 'str'
Created attachment 155189 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155190 [details] [review] DO NOT PUSH: Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Nind from comment #66) > Signed-off-by: David Nind <david@davidnind.com> Thx David for your quick signoff.
(In reply to David Nind from comment #67) > I had trouble attaching the patches because of the example plugin patch - I > hope I haven't screwed anything up! Not sure what was going wrong there exactly. But I restored the example patch. > Applying: DO NOT PUSH: Add example plugin > Moving to starting point > Adding bug reference 6b2e2e2 DO NOT PUSH: Add example plugin > Cleaning up back to original state on error > Traceback (most recent call last): > File "/usr/bin/git-bz", line 2834, in <module> > applied = do_apply(bug_ref) > File "/usr/bin/git-bz", line 1977, in do_apply > add_url(bug, commits) > File "/usr/bin/git-bz", line 1719, in add_url > add_url_to_head_commit(commit, bug) > File "/usr/bin/git-bz", line 1682, in add_url_to_head_commit > git.commit(file="-", amend=True, _input=input) > File "/usr/bin/git-bz", line 197, in f > o = git_run(command, *args, **kwargs) > File "/usr/bin/git-bz", line 167, in git_run > output, error = process.communicate(input) > File "/usr/lib/python3.9/subprocess.py", line 1134, in communicate > stdout, stderr = self._communicate(input, endtime, timeout) > File "/usr/lib/python3.9/subprocess.py", line 1961, in _communicate > input_view = memoryview(self._input) > TypeError: memoryview: a bytes-like object is required, not 'str' Hmm. I can reproduce this too now. Still looking a bit further.
Created attachment 155191 [details] [review] Bug 31503: DO NOT PUSH - Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #71) > Hmm. I can reproduce this too now. Still looking a bit further. Changed commit title. And cannot reproduce any more..
(In reply to Marcel de Rooy from comment #73) > (In reply to Marcel de Rooy from comment #71) > > Hmm. I can reproduce this too now. Still looking a bit further. > > Changed commit title. And cannot reproduce any more.. It now applies cleanly for me now as well - weird!
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc Comes from 27378. Waiting a bit for feedback on that report.
I just hoped that it would come further now, lets see ;)
This is a really cool feature.. I love the move to plugins as you can now really add functionality to the system and add a user consent step as part of that functionality improvement. I've rebased it locally and am playing around with it.. some small things I'd like to see to get it over the finish line, but actually I think it's really close now.
There are some system preference renames not accounted for yet in my local rebase. I'm working on those. I'm not sure I like how we've dropped the alert warning for privacy policy. I think it's important to highlight with that particular feature that the account is deleted and you are logged out if you do not accept and I think the reduction to 'just another consent' like the plugin one's your introducing here is a reduction of functionality. I'll work on a follow-up to keep it moving. Talking system preferences.. do you think libraries will want to independantly turn off the plugin based consents? I think perhaps that the system preference introduced is superflous to requirements. I think I'd just check for the occurrence of any plugins carrying the consent method instead.
Created attachment 155656 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155657 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155658 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155659 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155660 [details] [review] Bug 31503: (follow-up) Styling and pre-selection This patch restores some of the wording and styling from the original GDPR/Privacy policy feature including highlighting in a warning box. I also remove the jQuery approach for pre-selecting already selected radio options and rely on standard TT processing instead. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155661 [details] [review] Bug 31503: DO NOT PUSH - Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I've added a minor follow-up to restore a little of the original styling and wording from the Privacy Policy feature. Doing the above made me wonder if the 'description' display for plugin generated consent types should not be wrapped in <p>.. allowing for plugin authors to have more control over the final display of the description (for example adding a warning box as we do for the privacy policy one again now). I'm not 100% sure about the Koha::Patron->consent($type) accessor introduced here.. but I can see why you've done it that way and this patchset has been through a few iterations already.. so not a blocker. I'm also not in total agreement around the new system preference introduced. I believe you've done this for performance reasons but I tihnk we could work around it with caching instead.. again.. I don't think this should be a blocker. Finally.. and this one may be more of a blocker. I'm not sure how one would go about translating the description blocks introduced by plugins. We could pass language (or rely on context) through the available types method.. in fact, perhaps that's the answer.. we just leave it to the plugin author to look at lang from context and return the right thing when 'patron_consent_type' is called. I'd love to see this in the example plugin to highlight it's possible and works.. I may add that shortly if I can find a few more minutes. Generally, though, I'm happy with this and feel it can move forward. I'd love one final QA set of eyes.. I'll ask around for you.
(In reply to Martin Renvoize from comment #85) > Finally.. and this one may be more of a blocker. I'm not sure how one would > go about translating the description blocks introduced by plugins. We could > pass language (or rely on context) through the available types method.. in > fact, perhaps that's the answer.. we just leave it to the plugin author to > look at lang from context and return the right thing when > 'patron_consent_type' is called. I'd love to see this in the example plugin > to highlight it's possible and works.. I may add that shortly if I can find > a few more minutes. Thx Martin. Yes, I had a nl-NL key in my testing. Works fine. And will start using this approach in 22.11 soon. It feels like translation should be handled elsewhere. But we are waiting for long already on translation for authorised values, database translation, etc. So this is sufficient for now. Normally a local plugin will not contain many translations.
Created attachment 155691 [details] [review] Bug 31503: (follow-up) Add 'feature_enabled' to Koha::Plugins This patch adds a 'feature_enabled' accessor to the Koha::Plugins class. Passing the plugin method name for the plugin feature you're looking for will return a boolean denoting whether any enabled plugins carry said method. Incuded in the patch is an addition to the TT KohaPlugins plugin to enable quick access to this function.
OK, I've had a go at providing a performant method to allow us to drop the new system preference.. I'll continue on Monday.. I also think I've worked out the translation stuff, adding in translations for the descriptions into the plugin method shouldn't be too hard :) Thanks for your patience here Marcel.
Created attachment 155694 [details] [review] Bug 31503: Use KohaPlugins.feature_enabled in place of preference lookup
Created attachment 155783 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155784 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155785 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155786 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155787 [details] [review] Bug 31503: (follow-up) Styling and pre-selection This patch restores some of the wording and styling from the original GDPR/Privacy policy feature including highlighting in a warning box. I also remove the jQuery approach for pre-selecting already selected radio options and rely on standard TT processing instead. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155788 [details] [review] Bug 31503: (follow-up) Add 'feature_enabled' to Koha::Plugins This patch adds a 'feature_enabled' accessor to the Koha::Plugins class. Passing the plugin method name for the plugin feature you're looking for will return a boolean denoting whether any enabled plugins carry said method. Included in the patch is an addition to the TT KohaPlugins plugin to enable quick access to this function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155789 [details] [review] Bug 31503: (follow-up) Use KohaPlugins.feature_enabled This patch switches the code from using Koha.preference('OPACCustomConsentTypes') to using KohaPlugins.feature_enabled('patron_consent_type'). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155790 [details] [review] Bug 31503: (follow-up) Remove OPACCustomConsentTypes This patch removes all trace of the original OPACCustomConsentTypes preference. We now use Koha::Plugins->feature_enabled in preference. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 155791 [details] [review] Bug 31503: DO NOT PUSH - Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Martin Renvoize from comment #98) > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Ah great Martin. Thanks. Will have a look at your changes.
Does not apply anymore ):
# Failed test 'Article requests in 10 days' # at /usr/share/koha/t/db_dependent/Koha/Patron.t line 401. # got: '0' # expected: '1' # Looks like you failed 1 test of 19. Waiting for 35041 ;)
# Failed test 'Expect one type for privacy policy' # at /usr/share/koha/t/db_dependent/Koha/Patron/Consents.t line 59. # got: '2' # expected: '1' # Looks like you failed 1 test of 7. More trouble
WARN t/db_dependent/Koha/Patron/Consents.t WARN tidiness The file is less tidy than before (bad/messy lines before: 9, now: 10) WARN t/db_dependent/Koha/Plugins/Plugins.t WARN tidiness The file is less tidy than before (bad/messy lines before: 104, now: 110) Bah, dirty!
(In reply to Marcel de Rooy from comment #102) > # Failed test 'Expect one type for privacy policy' > # at /usr/share/koha/t/db_dependent/Koha/Patron/Consents.t line 59. > # got: '2' > # expected: '1' > # Looks like you failed 1 test of 7. > > More trouble Fixed in follow-up (In reply to Marcel de Rooy from comment #103) > WARN t/db_dependent/Koha/Patron/Consents.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 9, now: 10) > > WARN t/db_dependent/Koha/Plugins/Plugins.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 104, now: 110) > > Bah, dirty! Fixed inline
Created attachment 157117 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157118 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157119 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157120 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157121 [details] [review] Bug 31503: (follow-up) Styling and pre-selection This patch restores some of the wording and styling from the original GDPR/Privacy policy feature including highlighting in a warning box. I also remove the jQuery approach for pre-selecting already selected radio options and rely on standard TT processing instead. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157122 [details] [review] Bug 31503: (follow-up) Add 'feature_enabled' to Koha::Plugins This patch adds a 'feature_enabled' accessor to the Koha::Plugins class. Passing the plugin method name for the plugin feature you're looking for will return a boolean denoting whether any enabled plugins carry said method. Included in the patch is an addition to the TT KohaPlugins plugin to enable quick access to this function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> [EDIT] Tidied t/db_dependent/Koha/Plugins/Plugins.t [EDIT] Tidied Koha/Template/Plugin/KohaPlugins.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157123 [details] [review] Bug 31503: (follow-up) Use KohaPlugins.feature_enabled This patch switches the code from using Koha.preference('OPACCustomConsentTypes') to using KohaPlugins.feature_enabled('patron_consent_type'). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157124 [details] [review] Bug 31503: (follow-up) Remove OPACCustomConsentTypes This patch removes all trace of the original OPACCustomConsentTypes preference. We now use Koha::Plugins->feature_enabled in preference. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157125 [details] [review] Bug 31503: (QA follow-up) Improve count of available_types Tidied one other line. Test plan: Run t/db_dependent/Koha/Patron/Consents.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157126 [details] [review] Bug 31503: DO NOT PUSH - Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Tomas: Its yours again. Roadmap :)
RM: Needs DBIx schema change
Please rebase ASAP. Thanks!
(In reply to Tomás Cohen Arazi from comment #117) > Please rebase ASAP. Thanks! Heh Parse errors: Bad plan. You planned 29 tests but ran 30.
Created attachment 157773 [details] [review] Bug 31503: Change patron_consent.type Test plan: Run db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157774 [details] [review] Bug 31503: Change usermenu condition, add preference Adds preference OPACCustomConsentTypes. Test plan: Run dbrev. Toggle value of OPACCustomConsentTypes / PrivacyPolicyConsent. Check user page for Consents tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157775 [details] [review] Bug 31503: Make opac-patron-consent more generic Adds $patron->consent and $consents->available_types. Incorporates them into script/template. Provides two unit tests. Note: A follow-up patch helps you test this with an example plugin. Test plan: Run t/db_dependent/Koha/Patron.t Run t/db_dependent/Koha/Patron/Consents.t Toggle the value of pref PrivacyPolicyConsent and look at OPAC account, tab Consents. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157776 [details] [review] Bug 31503: (follow-up) Update description for system preference Update new OPACCustomConsentType system preference description - bold and capitalize NOTE for consistency, link to PrivacyPolicyConsent system preference for consistency, and other minor changes. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157777 [details] [review] Bug 31503: (follow-up) Styling and pre-selection This patch restores some of the wording and styling from the original GDPR/Privacy policy feature including highlighting in a warning box. I also remove the jQuery approach for pre-selecting already selected radio options and rely on standard TT processing instead. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157778 [details] [review] Bug 31503: (follow-up) Add 'feature_enabled' to Koha::Plugins This patch adds a 'feature_enabled' accessor to the Koha::Plugins class. Passing the plugin method name for the plugin feature you're looking for will return a boolean denoting whether any enabled plugins carry said method. Included in the patch is an addition to the TT KohaPlugins plugin to enable quick access to this function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> [EDIT] Tidied t/db_dependent/Koha/Plugins/Plugins.t [EDIT] Tidied Koha/Template/Plugin/KohaPlugins.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157779 [details] [review] Bug 31503: (follow-up) Use KohaPlugins.feature_enabled This patch switches the code from using Koha.preference('OPACCustomConsentTypes') to using KohaPlugins.feature_enabled('patron_consent_type'). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157780 [details] [review] Bug 31503: (follow-up) Remove OPACCustomConsentTypes This patch removes all trace of the original OPACCustomConsentTypes preference. We now use Koha::Plugins->feature_enabled in preference. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157781 [details] [review] Bug 31503: (QA follow-up) Improve count of available_types Tidied one other line. Test plan: Run t/db_dependent/Koha/Patron/Consents.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157782 [details] [review] Bug 31503: (QA follow-up) Fix number of tests in rebase Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157783 [details] [review] Bug 31503: DO NOT PUSH - Add example plugin Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Tomás Cohen Arazi from comment #117) > Please rebase ASAP. Thanks! Done.
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x
Switched to "Needs Documenting". Marcel, for documentation of this feature in the manual: Where would interested manual-readers find the example plugin? Can it just be installed like other plugins?
(In reply to Philip Orr from comment #133) > Switched to "Needs Documenting". > Marcel, for documentation of this feature in the manual: > Where would interested manual-readers find the example plugin? > Can it just be installed like other plugins? Last patch on this report. For a regular install it should be copied into a KPZ file. Documented on the wiki?
Could add note to the relevant section of the OPAC chapter, alongside the Privacy policy consent information. Actually, that sub-section doesn't yet exist at https://koha-community.org/manual/latest/en/html/opac.html#your-account It needs to be added!
Notes for me for testing: 1. Change enable_plugins to 1 (From the manual: To set up the Koha plugin system you must first make some changes to your install. Change <enable_plugins>0</enable_plugins> to <enable_plugins>1</enable_plugins> in your koha-conf.xml file. Restart your webserver) koha-conf.xml is in /etc. 2. install zip if not already installed: sudo apt-get install zip 3. make sure the Test_Newsletter.pm is in the following file structure: test/Koha/Plugin/Test_Newsletter.pm 4. From the test directory, do: zip -r koha-plugin-test-newsletter.kpz ./Koha 5. Now the plugin can be installed.
Marcel, sorry for the noob questions. I don't think I completely understand the test plan. I managed to create and upload the Test_Newsletter plugin, but I still don't have the syspref "OPACCustomConsentTypes". Do I need to do this part of the test plan first? Test plan: Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->install" Enable OPACCustomConsentTypes. Test consenting and refusing consent on OPAC account page. Run perl -MKoha::Plugin::Test_Newsletter -e"Koha::Plugin::Test_Newsletter->new->uninstall"
Oh, nevermind! Looking at the consent page in the OPAC answered my question. :)
Documentation finished in the merge request.