flake8
passed.
JSHint
passed.
Add decorator and mixin for requiring consent decisions
Review Request #9948 — Created May 15, 2018 and submitted
| Information | |
|---|---|
| brennie | |
| Djblets | |
| release-1.0.x | |
|
|
|
| 9976 | |
| eba94b3... | |
| Reviewers | |
| djblets | |
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_URLsetting.
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
failed.
JSHint
passed.
flake8
-
djblets/privacy/tests/test_consent_views.py (Diff revision 2) Show all issues
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+335 -1) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Check for correct setting, cleanup.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+339 -1) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
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
SimpleFormwhich is actually a view, and then we instantiate aSimpleFormthat's a form. Let's import aSimpleFormfrom 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)
flake8
passed.
JSHint
passed.
-
-
-
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.userout 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
failed.
JSHint
passed.
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)
flake8
passed.
JSHint
passed.
-
-
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_URLeverywhere :)
Change Summary:
Addressed feedback.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+364) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Addressed feedback.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+359) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.