Privacy policy link goes to a 403

Re

Going to a 403 seems worse than going to a stub page, maybe it should be published with the stub content?

This part was discussed also in 📌 Default content for privacy requirements Active . I agree that from the UX point, it might me better than 403, but if it will be published, it will be indexed by bots / search engines, which does not seems good, given the content it contains. And we know, that not all sites owners will immediately change the default content - sometimes it will be there for a long time, unfortunately.

Yes, this was the main reason to keep the node unpublished: this is at least a minimal enforcement on the site owner to take care of their privacy policy before going live with their site. This is related to the decision of the Drupal CMS product owners that we will not deliver any default content here. All the reasons behind that have been discussed and agreed in the linked issue by @poker10

Related to this, Klaro adds consent options for youtube and vimeo even if there's no content using those oembed providers.

We have been very careful in only enabling those services in consent management that are really necessary. As for YouTube and Vimeo this is because of the media type that support external videos, and that media type is available by default in Drupal CMS.

Can Klaro only be enabled with recipes that actually require a privacy policy?

That's what we believe is the case now.

Then those recipes could prompt for a privacy statement or link to the page via a tour.

That's what we're about to do. There is 📌 Add a tour for the "Privacy policy" page Active for the tour part of this. But since the tour module won't be ready for Drupal CMS 1.0, we want to add the necessary information into the Drupal CMS documentation pages - assuming that such a thing will be available.

We have been very careful in only enabling those services in consent management that are really necessary. As for YouTube and Vimeo this is because of the media type that support external videos, and that media type is available by default in Drupal CMS.

Could this be done programmatically when the first media item of that type is created? Seems like it might be doable with ECA?

Could this be done programmatically when the first media item of that type is created? Seems like it might be doable with ECA?

This is a smart idea, and yes, this could easily be done with ECA. However, this will then raise follow-up questions:

• If we only enable the external video apps when the first applicable media entity is being created, the initial setup of Drupal CMS will only have 2 apps enabled that are mandatory, i.e. no consent management is needed at all. We should then get Klaro to do nothing until the first app gets enabled that requires consent management, right?
• If the answer to the first question is yes, the menu item "Privacy settings" would have to disappear as well, until it's needed. Would it be OK to do this by keeping it in the menu, disabling it by default, and enabling it with ECA together with the first app that requires it?
• Do we also need to reverse mechanism, i.e. disable an app if the last remaining video media entity gets deleted? I hope not because that's not as straightforward as the other way round.

If we only enable the external video apps when the first applicable media entity is being created, the initial setup of Drupal CMS will only have 2 apps enabled that are mandatory, i.e. no consent management is needed at all. We should then get Klaro to do nothing until the first app gets enabled that requires consent management, right?

I wasn't sure about this, but if it's doable, then that would help significantly with out of the box front-end performance per 📌 Klaro significantly adds to anonymous visitor page weight Active .

If the answer to the first question is yes, the menu item "Privacy settings" would have to disappear as well, until it's needed. Would it be OK to do this by keeping it in the menu, disabling it by default, and enabling it with ECA together with the first app that requires it?

That seems reasonable.

Do we also need to reverse mechanism, i.e. disable an app if the last remaining video media entity gets deleted? I hope not because that's not as straightforward as the other way round.

Yeah that sounds hard and a much less likely case. It might be good to do that if an entire provider gets removed though - but this could be a follow-up if at all.

@catch I've implemented the ECA part in MR!305 and the Klaro part in the MR of ✨ Only add Klaro libraries when at least one relevant app is enabled Active . If you want to give that a try?

Tested the other issue by itself first.

Then left that MR applied and tested the one here. After a fresh install, there is only 5.6kb (compressed) JavaScript, this compared to 90kb or so before.

Seems to me that the changes here are orthogonal to the Klaro change itself - e.g. if this gets committed before CMS 1.0, sites will be correctly configured such that the upstream Klaro fix will just work when that's applied and released. Whereas if this waited, the sites would not get the benefit unless they manually make the same changes.

Given that, RTBC.

Actually not quite because I think the privacy settings link still needs to be hidden too?

Retitling.

No I was wrong twice - the link is disabled, just confused about which install I'm looking at :/

I think this should be moved to the drupal_cms_remote_video recipe. The privacy stuff is supposed to be "baked in", so it's shaping up to be the case that drupal_cms_privacy_basic is, like drupal_cms_content_type_base, one of those foundational recipes that others will take advantage of.

drupal_cms_events operates in a similar way.

A couple more asks:

• Since this moves module dependencies around, please update the module list: ddev list-modules > MODULES.md
• Can you file a follow-up to add functional test coverage?

Good call. So what should go to the remote video recipe is the ECA model which contains the magic.

But the re-configuration of the Klaro module and the menu link in the footer should remain in the privacy recipe, suppose.

I'll take care of the required actions tomorrow, as I'm running out of time for today.

That's fine.

But the re-configuration of the Klaro module and the menu link in the footer should remain in the privacy recipe, suppose.

Re: the menu link in the footer - I agree. I guess the question is, under what circumstances would we want sites to publish it?

I kind of think that maybe the privacy recipe should disable all Klaro apps by default and let the recipes that have privacy needs selectively enable them. Unless that would be overzealous?

@phenaproxima one problem with that just came to mind: the ECA model also enables the menu link in the footer when the first video entity gets created. That still works when this is part of the remote video recipe.

But we also need to enable that menu link in other scenarios, e.g. when the events recipe or the analytics recipe get applied. Is there a way to enable a menu link (=content entity) by a recipe?

Unfortunately, no. Recipes cannot do dynamic things to content; they can only import it and that's that.

Figuring that out is probably a follow-up. Maybe for now the best course of action is to leave the menu link enabled.

That would be a dead link, unfortunately. But I have an idea: we can use an ECA model that listens to config changes. And as soon as a relevant Klaro app gets enabled, ECA will enable the menu link. That should work, I'll give that a try tomorrow.

Oooh, I really like that! If it works, that's elegant as hell.

This would be a super smart solution!
Of course adds some "complexity" and things which needs to be kept in mind/tested, but when this works this is a really nice solution!

I've resolved all the threads in the MR and also moved the remote video ECA model to the other recipe. Then I added an ECA model to enable the menu link when a klaro app gets enabled that requires the user consent. That works as expected for videos. But there is one missing piece: when the event or analytics recipe gets applied, the klaro apps for leaflet or GA/GTM get enabled, but that doesn't dispatch the event for config save. Is that an issue in the recipe code?

Other than that, I've updated the MODULE.md file but there is no change. Looking into it, I saw that quite a few recipes are missing in that file, i.e. the remote video recipe is not in there.

Everything else works really nicely. Just hope that we get that missing event with the recipe config update resolved.

when the event or analytics recipe gets applied, the klaro apps for leaflet or GA/GTM get enabled, but that doesn't dispatch the event for config save. Is that an issue in the recipe code?

That's really weird. Let's handle that in a follow-up, and remove it from this issue for now, since it's not technically in scope.

Other than that, I've updated the MODULE.md file but there is no change. Looking into it, I saw that quite a few recipes are missing in that file, i.e. the remote video recipe is not in there.

I initially thought you meant you updated this manually, but you're right - the remote video recipe should be in there but isn't. That's a bug in the list-modules script, so I'll deal with that in a separate issue.

Beyond that, just needs test coverage I think! A PHPUnit functional test (maybe just add some stuff to the remote video ComponentValidationTest) would be perfectly sufficient.

Beyond that, just needs test coverage I think!

📌 Add performance testing Active would allow for some high level coverage of this.

After this is merged, the getScriptBytes() would be significantly reduced. Then we could enable one of the Klaro apps, hit the page again, and it would show all the extra JavaScript being loaded.

Might want more targeted test coverage of the actual functionality of course.

Just hope that we get that missing event with the recipe config update resolved.

This is solved !!! Guess what, I've made the most typical mistake when building an ECA model. It's been a permission issue, as drush commands get executed as anonymous user, changing the menu link entity is not permitted. I've had to explicitly change the user account so that this action can be performed. This is now completely working.

@phenaproxima the MODULES.md is now also properly updated.

Since this has privacy implications, I'm calling this a stable blocker.

Yes this should simultaneously improve out of the box performance (by taking Klaro out of the equation entirely) while also improving privacy overall (by automatically adding Klaro apps when relevant config is changed).

I have never actually used ECA, but it is frankly mind-blowing that a few lines of YAML is able to replace what would have been a fairly special purpose contrib module.

I've now added various tests:

• Privacy recipe: make sure that the footer menu is not visible. The, publish the privacy policy, and make sure it is then visible in the footer, but the privacy settings link is still not available.
• Analytics recipe: make sure the footer menu is visible and contains the privacy settings link, but not the privacy policy.
• Events recipe: make sure the footer menu is visible and contains the privacy settings link, but not the privacy policy.
• Remote video recipe: make sure the footer menu is not visible. Then, create a remote video media entity and make sure the footer menu will be visible with the privacy settings link but without the privacy policy

The tests are failing, but that looks like those random failures that are unrelated to the new tests.

@catch I'm not testing the presence of the javascript files from Klaro. Maybe that's meant to be present in your performance tests as soon as a new Klaro release is being tagged. The MR got merged, so we're half way in already.

Ah, re-ran the tests and all is now green.

@jurgenhaas yes we can adjust the performance tests once this is in. Or if the performance tests land first, tweak them here to show the improvement. But I until one lands no need to worry about the other yet.

Definitely doesn't need tests anymore!

I have a few minor nits but I'll fix those. Pretty sure this is RTBC otherwise as far as I'm concerned. Really nice work, and it's great to establish a pattern for doing this kind of thing. Go ECA!

Thank you for your help on this @phenaproxima, this is really great.

Just one ting that came to mind: as the remove_video recipe disables the Klaro apps for YouTube and Vimeo, there might be a theoretical risk that those get disabled again, if somebody re-applies that recipe some time after they had already created some remote video media entities. Turning the apps off at that point is not what should be happening. But I don't see how we could prevent that edge-case.

Idea: what if we changed the ECA model to react to a status change on the Klaro apps? So if something disables them, it immediately re-enables them if there are any published remote videos in the system.

What do you think?

Good thinking, I considered that too but see 2 potential issues:

Automatic re-enabling: that could be problematic if a site owner decides not to bother about privacy and disabling the apps manually. We should not prevent them from doing this.

Re-enable when at least one remote video is present on the site: that's an extra DB query and we shouldn't do that if not absolutely necessary. However, that wouldn't happen too often, only if we detected the state change first. So, maybe this is not a blocker.

The first one, however, would be an issue I guess. The only way around that would be that the ECA model would have to be disabled first before they disable the Klaro app. Hard to explain to a user, but maybe worth it?

if a site owner decides not to bother about privacy

This is a product-level decision and so should be run past @pameeela, but my knee-jerk reaction to this is that, a site owner decides they don't care about privacy, they are acting directly against what Drupal CMS is built to do (and, indeed, they might be violating the law depending on their circumstances).

Privacy is a core part of our offering, and if they decide they don't want it, we don't have to make it easy for them. We should make it possible for them to do whatever they want, of course -- and disabling the entire ECA model fits with that -- but that doesn't mean it needs to be easy for them to do things that we strongly think they shouldn't.

I can't disagree at all. And being a privacy advocate, this is music to my ears ;-) Let's see what @pameeela decides.

if a site owner decides not to bother about privacy

I think if they did, they would just uninstall Klaro altogether?

I think if they did, they would just uninstall Klaro altogether?

Probably yes, but there could be those who think that some configured privacy makes sense to them, but not the remote video part. But maybe I'm making up scenarios that will never happen.

While being on it, we managed here to reduce the overhead dramatically by only loading Klaro's javascript only when really necessary. At the same time, Jan Kellermann from Klaro found a way to reduce the size of the external jsvascript file from 208KB to 64KB uncompressed. This was possible by removing support for old browsers. I think, this is a fantastic achievement.

@phenaproxima In order to be prepared if @pameeela decides for that approach, I've added to the ECA model, that YouTube and Vimeo remain enabled once they have been enabled. If we will go into a different direction, we can quickly revert that commit. I just wanted to have everything in place so that it can go into RC2 when this gets tagged.

Assigning to @pameeela for final decision.

Re-applying recipes is not supported in the UI right now, and I think it is an edge case. But I also think the risk of accidentally disabling the apps is greater than the 'risk' of annoying site owners with privacy compliance. I guess we'll find out :)

Thanks! Merged into 1.x.

This broke HEAD for some reason.

I get this failure consistently (locally as well):

1) Drupal\Tests\drupal_cms_remote_video\Functional\ComponentValidationTest::test
Behat\Mink\Exception\ElementNotFoundException: Element matching css "nav > h2:contains("Footer") + ul" not found.

Fixed this in MR!323 and all tests are green again.

Problem was that our new feature to keep the Klaro app for remote videos always enabled prevented the recipe from disabling them. And because of that, the ECA model never enabled the menu link as it's supposed to do that only if the app got enabled after it was disabled.

I've fixed that such that the new feature to prevent disabling checks if there is at least one remote video and if not, it allows the app to be disabled.

I've fixed the changed label in the eca.eca.privacy_setting_link.yml file such that it also needs to be in eca.model.privacy_setting_link.yml and at the same time did another rebase.

As tests have been green already before, this seems ready to go.

At long last, merged into 1.x. Thanks!