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

Do not show cookies banner once user accept it #10884

Conversation

jibees
Copy link
Contributor

@jibees jibees commented May 24, 2023

Once the user accept the cookie, we should not show the cookie banner element. This is handled via app/helpers/footer_links_helper.rb#cookies_policy_link and boolean:

!Web::CookiesConsent.new(cookies, request.host).exists? && Spree::Config.cookies_consent_banner_toggle

As far as I know, this could not be revealed by automatic specs since we don't use cache in test env.

scenario "does not show after cookies are accepted" do
accept_cookies_and_wait
expect_not_visible_cookies_banner
visit_root_path_and_wait
expect_not_visible_cookies_banner
end

What? Why?

Closes #10882

What should we test?

  1. Delete all cookies, or use a private session
  2. Navigate between different pages in the shopfront.
  3. Accept the cookies.
  4. Note that the banner will be shown again and again.

Release notes

Changelog Category: User facing changes

Once the user accept the cookie, we should not show the cookie banner element. This is handled via `app/helpers/footer_links_helper.rb#cookies_policy_link` and boolean:

```
!Web::CookiesConsent.new(cookies, request.host).exists? && Spree::Config.cookies_consent_banner_toggle
```
@jibees jibees requested a review from Matt-Yorkley May 24, 2023 08:48
@jibees jibees self-assigned this May 24, 2023
@jibees jibees linked an issue May 24, 2023 that may be closed by this pull request
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented May 24, 2023

this could not be revealed by automatic specs since we don't use cache in test env.

Nice, thanks for that tip @jibees. Indeed, If we run that spec with:

config.action_controller.perform_caching = false

Set to true, the spec fails (in master) and passes with your code change ✨
I'll propose a spec in a separate PR.

(I'm still wondering where was this regression introduced...)

@jibees
Copy link
Contributor Author

jibees commented May 24, 2023

Thanks @filipefurtad0
We should include your suggestion on top of this PR. I could do it (I tried, not sure how to override this into the spec...)
This was probably introduced since #10772 I suppose.

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented May 24, 2023

I think I've found it @jibees.
(after trying this in several ways 😅)

It seems we have a caching: true tag for this, coming from our base spec helper:

# Enable caching in any specs tagged with `caching: true`.
config.around(:each, :caching) do |example|
caching = ActionController::Base.perform_caching
ActionController::Base.perform_caching = example.metadata[:caching]
example.run
ActionController::Base.perform_caching = caching
end

We should probably run the whole spec with caching, right?

I've enabled it here a22fa19 / #10887.

@jibees
Copy link
Contributor Author

jibees commented May 24, 2023

Thanks! I've cherry picked your commit!

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 👍

@filipefurtad0 filipefurtad0 self-assigned this May 25, 2023
@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels May 25, 2023
@@ -96,7 +96,7 @@
%hr.hr-light
%br

= cache_with_locale [ContentConfig.cache_key, TermsOfServiceFile.current_url, Spree::Config.privacy_policy_url] do
= cache_with_locale [ContentConfig.cache_key, TermsOfServiceFile.current_url, Spree::Config.privacy_policy_url, Spree::Config.cookies_consent_banner_toggle, Web::CookiesConsent.new(cookies, request.host)] do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR:
I wonder if we should enable cache for spec/services/terms_of_service_spec.rb...

@filipefurtad0
Copy link
Contributor

Hey @jibees ,

Thanks for this quick fix. I've staged the PR and can confirm that, by following the steps you've pointed out:

  • After deleting all cookies, or using a private session
  • The cookie banner is displayed when visiting the shopfront
  • After accepting the cookie policy, the banner is not displayed again

Some regression testing:

  • logged in
  • verified that the session is persistent
  • checked out

All good, merging! 🚀

PS: signalling issue #10261, as I've bumped into it during these tests:

image

@filipefurtad0 filipefurtad0 merged commit 8b7ca29 into master May 25, 2023
@filipefurtad0 filipefurtad0 deleted the 10882-cookie-banner-is-displayed-on-every-page-even-after-accepting branch May 25, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie banner is displayed on every page even after accepting
3 participants