Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#2907 fix for gdpr clobering contact tokens #21812

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 12, 2021

Overview

This is a fix for contact tokens not rendering when gdpr tokens is installed - https://github.com/veda-consulting-company/uk.co.vedaconsulting.gdpr

Gdpr declares some addtional tokens with the namespace 'contact'. I am inclined to
agree with @magnolia61's suggestion it is 'naughty' and so while I have fixed the clobbering by
not processing contact-metadata tokens I have not fixed the enotices.

If we think it is non-naughty I can kill those - but I've already loosened
some strictness that I think had benefits in order to facilitate this

Before

Contact tokens don't render with gdpr extension installed. This extension declares tokens in the 'contact' namespace such as {contact.comm_pref_supporter_link}

After

image

Technical Details

Comments

Thanks for the testing / diagnosis @magnolia61

@civibot
Copy link

civibot bot commented Oct 12, 2021

(Standard links)

@magnolia61
Copy link
Contributor

magnolia61 commented Oct 12, 2021

Using this patch solved the problem partially:
afbeelding

With the gdpr extension disabled:
afbeelding

fields like birthday, city and email still didn't work with this patch. Will try to test some more

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 thanks - I'll test those fields specifically

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 I think I've fixed it

@magnolia61
Copy link
Contributor

@eileenmcnaughton you've fixed it

@eileenmcnaughton
Copy link
Contributor Author

yay - @colemanw can you MOP this based on testing - it's an rc regression

@eileenmcnaughton
Copy link
Contributor Author

although @colemanw I should note there is a question about the 'naughty & nice' thing - around e-notices

@seamuslee001
Copy link
Contributor

so @eileenmcnaughton I suppose this begs the question but is probably answered by your PR description but are we explicitly meant to be blocking people from using contact, event, participant etc categories in their extensions e.g. coming up with their own custom tokens or is it just that GDPR is doing something rather badly?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 yeah - so this change means that it will work if they do that - but there will be e-notices. I've landed on 'discourage but don't block' - ie let the notices happen

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I just tested with civitoken which polutes the 'contribution' namespace in tokens (although it's a bit different since hookTokens is really about contact tokens) and the notices only apply to pollution of the contact namespace

This is a fix for contact tokens not rendering when gdpr tokens is installed.

Gdpr declares some addtional tokens with the namespace 'contact'. I am inclined to
agree this counts as 'naughty' and so while I have fixed the clobbering by
not processing contact-metadata tokens I have not fixed the enotices.

If we think it is non-naughty I can kill those - but I've already loosened
some strictness that I think had benefits in order to facilitate this
@magnolia61
Copy link
Contributor

magnolia61 commented Oct 13, 2021

first_name, last_name, birth_date and job_title still do not seem to render in combination with the gdpr extension enabled

@eileenmcnaughton
Copy link
Contributor Author

@magnolia61 I just tested this from contact search + pdf letter task with gdpr enabled & the 4 fields rendered

image

@magnolia61
Copy link
Contributor

With me these 4 fields (first_name, last_name, birth_date and job_title) still do not render when gdpr is installed.
I am using scheduled reminders to test that.

But I do not think it should be fixed here, I think in a new release GDPR could adopt to a better way of token-handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants