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

feat: check cookie consent when muting game #1157

Merged
merged 3 commits into from
Apr 16, 2021
Merged

Conversation

razvan-pro
Copy link
Collaborator

@razvan-pro razvan-pro commented Jan 13, 2021

Description

When muting the game, the cookie should only be set if the user gave consent for functional cookies. Muting the game should still work, but the status won't be saved on page reload or other levels if the cookie is not set.

How Has This Been Tested?

  1. Change cookie settings to allow functional cookies, then mute the game and either refresh the page or go to another level. The game should still be muted.
  2. Change cookie settings to disallow functional cookies, then mute the game and either refresh the page or go to another level. The game should not be muted anymore as the status has not been saved. However, toggling the mute button should work while on the page (until reloading).

Checklist:

  • Game mute status should be saved if the user gives functional cookies consent.
  • Game mute status should not be saved otherwise.
  • Game mute feature should still work in any case.

This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #1157 (c80b385) into master (c041935) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   91.31%   91.30%   -0.02%     
==========================================
  Files         111      111              
  Lines        6197     6197              
==========================================
- Hits         5659     5658       -1     
- Misses        538      539       +1     
Impacted Files Coverage Δ
game/random_road.py 90.64% <0.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c041935...c80b385. Read the comment docs.

@razvan-pro razvan-pro self-assigned this Jan 15, 2021
Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @razvan-pro)

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @razvan-pro)


game/static/game/js/game.js, line 489 at r1 (raw file):

  } else {
    ocargo.sound.unmute()
    if (hasFunctionalCookiesConsent()) {

Since CodeClimate is complaining that this bit is duplicate code with the bit above and that the function's cyclomatic complexity is too high, maybe you could refactor this into a separate function and pass in the true/false argument?

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @razvan-pro)

Copy link
Collaborator Author

@razvan-pro razvan-pro left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


game/static/game/js/game.js, line 489 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Since CodeClimate is complaining that this bit is duplicate code with the bit above and that the function's cyclomatic complexity is too high, maybe you could refactor this into a separate function and pass in the true/false argument?

👍

@razvan-pro razvan-pro marked this pull request as ready for review April 14, 2021 16:22
Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @razvan-pro)

@razvan-pro razvan-pro merged commit 85b5279 into master Apr 16, 2021
@razvan-pro razvan-pro deleted the managed-cookies branch April 16, 2021 12:11
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.

2 participants