Not a big fan of the tests. But I need to take a deeper look if I got the time. Nothing seems to directly check the menu link attributes. Only permission and settings form tests...
Added a few comments for now.
Is this ready for review? Status is still set to "Needs work", but tests were added. Also for some reason the test pipeline did not start testing?
I know this is quite unrelated, but also remove Drupal 8 and 9 support. We should allow 10.5 as minimum.
Tests are not failing upstream. Only phpcs. And here phpunit succeeds, but we have failures for phpunit_next_minor and phpunit_next_major.
Possible, that the tests haven't run on dev for a while now, that's why we don't see them on the main repo.
Created a new release: https://www.drupal.org/project/ad/releases/11.0.0-alpha6 → .
Alright, this works great together with 📌 Attempt to break part tour.js Active !
Just one minor adjustment, we need to use "tour.tour_settings" as the base route, otherwise the translation tab won't be created. I manually adjusted the weight, so it looks how it looked before:
EDIT: Sorry wrong screenshot.
That's it. RTBC!
This works perfectly and fixes our issue from 🐛 Tour strings are not translatable, should be "label" in schema Needs work ! Thanks @smustgrave!
Code changes LGTM as well! RTBC!
I really don't understand the issue, especially since it is always referencing cdn.js...
We are not conditionally adding the cdn.js anywhere! It is always set as a dependency of init.js. If it were for init.js then "posthog_js_page_attachments" with its conditionChecker check, could be the problem. But it's only cdn.js!
I'll provide a patch entirely removing "hook_library_info_alter()", which we can test online first, since we are using CDN anyway. This is quite the shot in the dark, but I have no other idea.
Yea, all in all this LGTM! RTBC!
Are you able to merge this @vmarchuk? Once this is merged, I can add the PayPal support in 📌 Add PayPal support to Payment Element Gateway Active .
@vmarchuk I can't reproduce this behavior.
Went through the standard checkout, then back to the cart and bought the product through the express checkout while debugging. The ExpressCheckoutController, "onPaymentConfirm" clears the checkout data as expected before continuing, and you can even see that reflected in the stripe logs:
I created a new [...]-2.x branch with a new MR, based on the original issue branch.
grevil → changed the visibility of the branch 3421392-add-paypal-support to hidden.
grevil → changed the visibility of the branch 3477028-stripe-express-checkout-with-3421392-add-paypal-support to hidden.
Hey @vmarchuk!
For us, it is working great! I created a branch combining this issue together with 📌 Add PayPal support to Payment Element Gateway Active , which I just did some successful testing on! I'll check on the MR comments again and try to highlight the important once.
we need to clear the checkout data set during regular checkout when the pop-up opens
I have only tested express and regular checkout in isolation with this patch. What exactly are the steps to reproduce? Do you mean going through the express checkout, then cancelling it and going into the regular checkout?
Here is a 3477028 x 3421392 "franken-patch" for the time being.
Also, there were no specific tests for this issue added... but I really don't wanna waste more time here. Please review.
Added tests. Although, one will fail, because of 📌 JS advertisement type does not yet actually run the JS Active .
Not really worth it using AI to generate the tests. Took too many manual adjustments and therefore too much time...
Might need a better prompt.
One test will fail, because ad_content_js doesn't execute js. See 📌 JS advertisement type does not yet actually run the JS Active .
Pipeline display is stuck. Pipeline is green: https://git.drupalcode.org/project/jsonapi_role_access/-/pipelines/666020. Merging....
All right, all done. Quite a bit of refactoring, but everything is pretty straigth forward, except the removal of the javascript, but hear me out on this one.
Conditionally hiding / showing checkboxes has many problems:
- Roles that are checked, but hidden through js still exist in config.
- Error prone, (see 🐛 Role hiding assumption is too optimistik Active ).
- Too opinionated: anonymous, authenticated and admin roles can be whatever the website creator wants it to be. This module only covers the default configuration for these roles.
Also, the user is now also able to select the "administrator" role. Unsure, why this was filtered out from the options.
Please review!
Ok, that should be it. I'll do some code refactoring in a follow-up issue.
Nice, the test doesn't fail at line 76 anymore, now it fails at line 86. I will continue there tommorow.
I think the main issues were, that the authorization header was missing and the route builder did not rebuild after we created our content type.
But I'll leave the rest of the changes in as well, so the tests are more in line with the core json_api tests.
LGTM, and works as expected, just added a few comments regarding the tests.
Remaining test failures should be fixed in a follow-up issue. Otherwise there will be too many unrelated changes here.
Please review!
For @anybody and I the problem was the "Watchdog Mailer" module.
We injected the "@date.formatter" (DateFormatter.php) service into the logger. But loggers seem to run way too early for "DateFormatter", resulting in this issue.
Here is the related issue: 📌 "WatchdogMailer" logger runs too early for @date.formatter to be injected, causing a fatal error on cache invalidation. Active .
Otherwise, this looks great! Thanks, @igor mashevskyi!
Both hook_entity_insert() and hook_entity_presave() now have "Order::First" set. Those are the hooks which caused issues back in 🐛 Set Label runs two times on node creation Needs work .
I'll test it locally now.
I don't really like this approach. Part of OOP Hooks is code clarity and simply putting all .module file hooks into one Hook class simply delegates the original problem of hooks inside a .module file into this class.
Here is the follow-up issue: #3558230: Fix "testCookiesBanner" test not succeeding → .
The last test is definitely unrelated.
I tried to reproduce the behavior in the tests. Probably a race condition, since we never wait in the simple "testCookiesBanner" test. I'll merge this as is. We can create a follow-up issue, if we want.
Whoa, good thing we have the tests. My previous changes caused gtag.js to always be knocked, no matter what.
I just auto formatted the code and moved the for... in... loops to ".forEach", seems like this caused some issues. Probably a "this" that now has a different context. Anyway, I reverted the eslint changes again and tested manually locally. Works again.
This might already do the trick.
Now the logic is in line with page_attachements / _attachements_alter.
Interesting. The two scripts causing us problems (init.js from posthog and gtag.ajax) are present in the body after consent denied. Furthermore, gtag.ajax is not even loaded on initial load.
Only when COOKiES is installed, that library appears on the main page. Otherwise, its not even loaded. I wonder why.
Yes it works as documented:
No events are captured until after consent is either given or denied
then on consent denied:
Opting out on a PostHog client will prevent all data from being captured and sent to PostHog.
Ok, so it does work.
When giving consent, the cookie is set and everything is tracked as expected and if consent is denied, no cookie is set and no client events get tracked (server events still get tracked anonymously).
But this is not as expected. As the description of the cookieless tracking says:
- No events are captured until after consent is either given or denied
- If consent is denied, still counts those users, using a privacy-preserving hash calculated on PostHog's servers
The second note is simply wrong, or at least they don't describe HOW to deny consent. My guess is they mean posthog.opt_out_capturing() but in the description of that method it says:
Opting out on a PostHog client will prevent all data from being captured and sent to PostHog.
Maybe the wording is bad in the first blockquote, and they literally only count the user when consent is denied and don't track anything else.
I guess we are not allowed to track the user without consent even if he is anonymized. At least the server events are still being registered. Unsure if that is enough @anybody. You decide.
Note, that I already tried manually setting cookieless_mode: 'on_reject' inside the init.js and use callback_code: "if (consent == true) {\r\n window.posthog.opt_in_capturing()\r\n}\r\n"
So that we never opt out of consent, but as documented, this won't track any events.
Same with if (consent == true) {\r\n window.posthog.opt_in_capturing()\r\n}\r\nelse {\r\n window.posthog.opt_out_capturing()\r\n}\r\n
Interestingly enough, if we entirely remove the cookieless_mode: 'on_reject' (everywhere) and only use Same with if (consent == true) {\r\n window.posthog.opt_in_capturing()\r\n}\r\nelse {\r\n window.posthog.opt_out_capturing()\r\n}\r\n the cookie with the distinct id gets set, but nothing gets tracked.
So I guess the current MR is as good as it gets. Please do a final review @anybody.
I adjusted the implementation a bit. The special thing with posthog is, that we don't really need to block anything. Not the JS nor the cookies.
The JS is GDPR compliant from the get go and there is a special setting that will only set the cookie and activate the tracking, once the user agrees to the Cookie Banner (see https://posthog.com/docs/privacy/data-collection).
The only thing is, that the code from the MR probably won't be enough, as the "window.posthog.set_config({ cookieless_mode: 'on_reject' });" runs to late, as it needs to be run on the posthog init.
I don't think that will be enough. I'll check this, once I'll take a look at 💬 Does this implement "cookieless_mode"? Active .
We probably won't ever need this functionality, but if anyone needs it, feel free to create a merge request!
All green again! There was a little bug, but all good again :)
I'll do some final testing and then we should be good to go!
Ok, there is still an update hook needed and the functionality needs to be tested.
Otherwise this is good to go.
This now got a bit more important, because of 💬 How to get gclid into Posthog PHP conversion? Active .
Alright, this does the trick.
I added the cache context to the variation entity class itself, to be in line with the "Product" entity cache context:
public function getCacheContexts() {
return Cache::mergeContexts(parent::getCacheContexts(), ['url.query_args:v', 'store']);
}
(modules/product/src/Entity/Product.php, line 293-295)
I'd say this won't introduce any issues, since the context is only valid, if the parameter exists in the url. But if one of the maintainers sees problems with this approach, we need to manually set the cache context in the template_preprocessing function or use an alternative approach all together.
Anyway, please review!