Add decorator and mixin for requiring consent decisions

Review Request #9948 — Created May 15, 2018 and submitted

Information

Djblets
release-1.0.x
eba94b3...

Reviewers

We now provide a view decorator (@check_pending_consent) and a view
mixin (CheckPendingConsentMixin) for requiring all consent decisions
to be made. If said decisions are not all made, the user will be
redirected to the URL specified in the
DJBLETS_PRIVACY_PENDING_CONSENT_REDIRECT_URL setting.

Ran unit tests.

Description From Last Updated

F401 'djblets.cache.backend.make_cache_key' imported but unused

reviewbotreviewbot

Too many blank lines.

daviddavid

Doc comment and superclass say returns but this doesn't have a return statement.

daviddavid

n before r

daviddavid

t before v

daviddavid

t before v

daviddavid

This example is a little confusing. We have a class called SimpleForm which is actually a view, and then we …

daviddavid

Alphabetical order.

chipx86chipx86

:setting: isn't a thing. There's no reference or role for settings. For now, just do settings.BLAH.

chipx86chipx86

This should prefix settings. to add some context.

chipx86chipx86

We should allow this to be a function, since the URL may very well depend on the request.

chipx86chipx86

Too many blank lines. Same comment about :setting:.

chipx86chipx86

This should be one line. Why are we even taking request? Seems unnecessary. The decorator will still get it.

chipx86chipx86

This should just use record_consent_data_list(), and then there's no need to pull request.user out into user (we'll only access it …

chipx86chipx86

Should be: ``settings.DJBLETS_PRIVACY_CONSENT_TRACKER``

chipx86chipx86

Let's add an anchor here.

chipx86chipx86

Same.

chipx86chipx86

Feels like this is too complex an example. It looks like a form is required here. Let's keep it as …

chipx86chipx86

These aren't valid responses for a view.

chipx86chipx86

The extra blank line can go away.

chipx86chipx86

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Doesn't this redirect to settings.DJBLETS_PRIVACY_PENDING_CONSENT_REDIRECT_URL? That said, I'd actually be OK calling it PENDING_CONSENT_REDIRECT_URL everywhere :)

daviddavid

This should no longer be in "Args".

chipx86chipx86

No blank line here.

chipx86chipx86

This should take **kwargs.

chipx86chipx86

No trailing blank line.

chipx86chipx86
brennie
Review request changed
Commit:
657c8766854b8976df6bfdd2d754fda761aaff17
8e68c0f72028a7a01516d94966a9a28938b58835

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. djblets/privacy/consent/views.py (Diff revision 4)
     
     
     
    Show all issues

    Too many blank lines.

  3. djblets/privacy/consent/views.py (Diff revision 4)
     
     
     
    Show all issues

    Doc comment and superclass say returns but this doesn't have a return statement.

  4. djblets/privacy/tests/test_consent_views.py (Diff revision 4)
     
     
     
    Show all issues

    n before r

  5. djblets/privacy/tests/test_consent_views.py (Diff revision 4)
     
     
     
     
    Show all issues

    t before v

  6. djblets/privacy/tests/test_consent_views.py (Diff revision 4)
     
     
     
    Show all issues

    t before v

  7. docs/djblets/guides/privacy/consent.rst (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    This example is a little confusing. We have a class called SimpleForm which is actually a view, and then we instantiate a SimpleForm that's a form. Let's import a SimpleForm from some nonexistant place, and then call the class itself SimpleView.

  8. 
      
brennie
chipx86
  1. 
      
  2. djblets/privacy/consent/views.py (Diff revision 5)
     
     
     
     
    Show all issues

    Alphabetical order.

  3. djblets/privacy/consent/views.py (Diff revision 5)
     
     
    Show all issues

    :setting: isn't a thing. There's no reference or role for settings. For now, just do settings.BLAH.

  4. djblets/privacy/consent/views.py (Diff revision 5)
     
     
    Show all issues

    This should prefix settings. to add some context.

  5. djblets/privacy/consent/views.py (Diff revision 5)
     
     
    Show all issues

    We should allow this to be a function, since the URL may very well depend on the request.

  6. djblets/privacy/consent/views.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Too many blank lines.

    Same comment about :setting:.

  7. djblets/privacy/consent/views.py (Diff revision 5)
     
     
     
    Show all issues

    This should be one line.

    Why are we even taking request? Seems unnecessary. The decorator will still get it.

  8. djblets/privacy/tests/test_consent_views.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This should just use record_consent_data_list(), and then there's no need to pull request.user out into user (we'll only access it once).

    We can also get rid of the tracker = get_consent_tracker(), just doing get_consent_tracker().record_consent_data_list(...).

    Same comments apply to other tests.

  9. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
    Show all issues

    Should be:

    ``settings.DJBLETS_PRIVACY_CONSENT_TRACKER``
    
  10. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
     
    Show all issues

    Let's add an anchor here.

  11. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
    Show all issues

    Same.

  12. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Feels like this is too complex an example. It looks like a form is required here. Let's keep it as simple as the plain view.

  13. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
     
     
     
    Show all issues

    These aren't valid responses for a view.

  14. docs/djblets/guides/privacy/consent.rst (Diff revision 5)
     
     
     
    Show all issues

    The extra blank line can go away.

  15. 
      
brennie
Review request changed
Change Summary:

Addressed feedback.

Commit:
b8eeb8086a7ea2aca7c7d02c78b9a4ee09b25eaa
4e6b0c0c84b98a5f5e9153968575ed95854ec3a5

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. djblets/privacy/consent/views.py (Diff revision 7)
     
     
    Show all issues

    Doesn't this redirect to settings.DJBLETS_PRIVACY_PENDING_CONSENT_REDIRECT_URL?

    That said, I'd actually be OK calling it PENDING_CONSENT_REDIRECT_URL everywhere :)

  3. 
      
brennie
chipx86
  1. 
      
  2. djblets/privacy/consent/views.py (Diff revision 8)
     
     
     
     
    Show all issues

    This should no longer be in "Args".

  3. docs/djblets/guides/privacy/consent.rst (Diff revision 8)
     
     
     
     
    Show all issues

    No blank line here.

  4. docs/djblets/guides/privacy/consent.rst (Diff revision 8)
     
     
    Show all issues

    This should take **kwargs.

  5. docs/djblets/guides/privacy/consent.rst (Diff revision 8)
     
     
     
    Show all issues

    No trailing blank line.

  6. 
      
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (0b169b8)