Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Restrict media privacy until a referencing page is published #46
base: main
Are you sure you want to change the base?
Restrict media privacy until a referencing page is published #46
Changes from 42 commits
6fb93bd
9f912b9
8aad840
b30bd3b
4dd76f3
12a1480
97215de
ea7466c
592807f
1c34a50
fb73cb3
92613b3
c73cd07
04e31c2
ab6a27d
7f7cb4f
19dde02
f7c46e0
6fc3c98
5919d83
22007bb
b3fa22e
8b19623
e1ecdb3
0621789
585f90f
52267f2
10fdba7
f67ac65
8037f3d
f0c5d04
aa86f2d
439d5a1
0a31a10
a2748a6
f13bb20
22e3f8a
bef76b4
a42a9c5
c16b886
fe260b8
593af11
3a66cd0
ce09c50
16a05fb
60fdb15
2f5dd30
dee1193
50f131d
f3af123
b581823
7f0de81
5198943
89a670f
d5f2ecc
faefcac
0bf2898
c196dee
e164276
aaea1e9
cfe3672
6284741
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For private media, I would like to enforce the fact that the storage backend must have these handlers. For local dev, the handles can "log out the action". I would rather not have it pass silently if storage doesn't support it; I would like to take the approach of the storage backend must implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnily enough, that's how I had it originally, but I changed my mind at some point. Should be very easy to restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 16a05fb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, could be something shorter like:
MEDIA_PERMISSION_MAX_WORKERS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to stick with the
PRIVATE_MEDIA_
prefix, as it makes it easier to mentally map settings to apps in the project. But, I do find the current name a little confusing, so have changed toPRIVATE_MEDIA_BULK_UPDATE_MAX_WORKERS
in ce09c50.Whilst it's still a bit lengthy, I think clarity is more important when it comes to settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, add as class attr like the
publish_bundles.py
command does instead of the pylint disable?. The key point is consistency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I think it is easier to read if we added some newlines and exit early to reduce indentation. It feels a bit crowded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnily enough, that's how I had it originally, but it was before I broke handling off to
update_file_permissions
, and pylint had a problem with the number of return statements. It should be okay to do that again now thatupdate_file_permissions
existsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 7f0de81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type annotation for args should be abstract unless the method needs it to be a
list
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree where it comes to reusable code, but in the case of management commands (which are self-contained, and only really reusable if you're defining a base class for other commands to inherit), I think being explicit adds value. Less... 'how you might use me' and more 'this is exactly how i'm being used'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 5198943