Gutenberg: Remove Upload Options for Users Who Lack Permission #18229
Conversation
| fileprivate extension Blog { | ||
| var userCanUploadMedia: Bool { | ||
| // Self-hosted non-Jetpack blogs have no capabilities, so we'll just assume that users can post media | ||
| return capabilities != nil ? isUploadingFilesAllowed() : true | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
👋 @frosty ! I was wondering if you might have any thoughts on whether my decision to move this logic into Blog+Capabilities is sound. I did this to avoid duplicating the logic both here and with the GutenbergViewController since the capabitlies != nil part of the check seemed like a tricky little gotcha, but I wonder if there's a better way to do that. Or perhaps this small bit of duplication isn't worth avoiding.
Adding something to Blog+Capabilities just felt like it might not be the right decision, and I'm hoping to get some input from someone who is more familiar with Swift and WPiOS than I am (admittedly, this is a low bar). You added this method, so that's why I pinged you, but It has been almost 5 years since you did that, so I'm going to go out on a limb and guess that it might not be fresh in your mind. 🤣 Just let me know if you don't have time to take a look at this or if you think there might be another person who would be better to ping.
If you'd like to see the refactor in isolation you can look at af5a94a.
There was a problem hiding this comment.
Hey, apologies for missing the ping! I think this seems like a good location for this code, as it fits in with the other capabilities checks. I appreciate the comment in there too to explain why we're counting nil capabilities as true.
It bothers me slightly that the name userCanUploadMedia doesn't match the format of the other methods here (isXAllowed), but I can't really think of a better name that does match! So I think it's fine. Could we add a /// doc comment to the method, like the others in here?
Thanks for checking!
There was a problem hiding this comment.
Thanks for taking a look @frosty ! 🙇
It bothers me slightly that the name userCanUploadMedia doesn't match the format of the other methods here (isXAllowed), but I can't really think of a better name that does match!
I agree, this name definitely isn't great. In addition to not matching the format of the other names, my addition makes it so that we have two methods (isUploadingFilesAllowed and userCanUploadMedia) whose names make it sound like they both do the same thing. I'm definitely open to suggestions for a better name if you or anyone else has any ideas.
Thinking about this some more, I'm starting to think that this might actually pointing to a problem with the non-optional boolean return value for these capability methods. What if instead of returning a boolean they returned an enum along the lines of
enum Capability {
case capable, notCapable, unknown
}
That would make the method signature more informative and hopefully cue the user of the method to think about how they want to handle the situation where we don't know whether the user has a given capability. That might be wishful thinking though since it would be easy to just check if something was == Capability.capable without giving any thought to the unknown case.
This is mostly me just thinking out loud. If you think this might be a good idea, it's probably something I would prefer to do as a separate PR since it would probably end up causing small changes to a lot of files.
|
👋 I'm going to close this PR due to lack of activity, but please reopen if this was an error. Thank you. |
See the related Gutenberg PR for a description of these changes and test steps.
Regression Notes
No unintended areas of impact come to mind.
PR submission checklist:
RELEASE-NOTES.txtif necessary.