Add decorator and mixin for requiring consent decisions
Review Request #9948 — Created May 15, 2018 and submitted
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 |
reviewbot | |
Too many blank lines. |
david | |
Doc comment and superclass say returns but this doesn't have a return statement. |
david | |
n before r |
david | |
t before v |
david | |
t before v |
david | |
This example is a little confusing. We have a class called SimpleForm which is actually a view, and then we … |
david | |
Alphabetical order. |
chipx86 | |
:setting: isn't a thing. There's no reference or role for settings. For now, just do settings.BLAH. |
chipx86 | |
This should prefix settings. to add some context. |
chipx86 | |
We should allow this to be a function, since the URL may very well depend on the request. |
chipx86 | |
Too many blank lines. Same comment about :setting:. |
chipx86 | |
This should be one line. Why are we even taking request? Seems unnecessary. The decorator will still get it. |
chipx86 | |
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 … |
chipx86 | |
Should be: ``settings.DJBLETS_PRIVACY_CONSENT_TRACKER`` |
chipx86 | |
Let's add an anchor here. |
chipx86 | |
Same. |
chipx86 | |
Feels like this is too complex an example. It looks like a form is required here. Let's keep it as … |
chipx86 | |
These aren't valid responses for a view. |
chipx86 | |
The extra blank line can go away. |
chipx86 | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
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 :) |
david | |
This should no longer be in "Args". |
chipx86 | |
No blank line here. |
chipx86 | |
This should take **kwargs. |
chipx86 | |
No trailing blank line. |
chipx86 |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+336 -1) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/privacy/tests/test_consent_views.py (Diff revision 2)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+335 -1) |
Checks run (2 succeeded)
Change Summary:
Check for correct setting, cleanup.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+339 -1) |
Checks run (2 succeeded)
-
-
-
djblets/privacy/consent/views.py (Diff revision 4) Doc comment and superclass say returns but this doesn't have a return statement.
-
-
-
-
docs/djblets/guides/privacy/consent.rst (Diff revision 4) This example is a little confusing. We have a class called
SimpleForm
which is actually a view, and then we instantiate aSimpleForm
that's a form. Let's import aSimpleForm
from some nonexistant place, and then call the class itselfSimpleView
.
Change Summary:
Addressed David's feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+342 -1) |
Checks run (2 succeeded)
-
-
-
djblets/privacy/consent/views.py (Diff revision 5) :setting:
isn't a thing. There's no reference or role for settings. For now, just dosettings.BLAH
. -
djblets/privacy/consent/views.py (Diff revision 5) This should prefix
settings.
to add some context. -
djblets/privacy/consent/views.py (Diff revision 5) We should allow this to be a function, since the URL may very well depend on the request.
-
djblets/privacy/consent/views.py (Diff revision 5) Too many blank lines.
Same comment about
:setting:
. -
djblets/privacy/consent/views.py (Diff revision 5) This should be one line.
Why are we even taking
request
? Seems unnecessary. The decorator will still get it. -
djblets/privacy/tests/test_consent_views.py (Diff revision 5) This should just use
record_consent_data_list()
, and then there's no need to pullrequest.user
out intouser
(we'll only access it once).We can also get rid of the
tracker = get_consent_tracker()
, just doingget_consent_tracker().record_consent_data_list(...)
.Same comments apply to other tests.
-
docs/djblets/guides/privacy/consent.rst (Diff revision 5) Should be:
``settings.DJBLETS_PRIVACY_CONSENT_TRACKER``
-
-
-
docs/djblets/guides/privacy/consent.rst (Diff revision 5) 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.
-
-
Change Summary:
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+364) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/privacy/consent/views.py (Diff revision 6) E126 continuation line over-indented for hanging indent
Change Summary:
Flake8 fixups
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+364) |
Checks run (2 succeeded)
-
-
djblets/privacy/consent/views.py (Diff revision 7) 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 :)
Change Summary:
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+364) |
Checks run (2 succeeded)
Change Summary:
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+359) |