-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Current user capabilities #3991
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
Merged
maxme
merged 14 commits into
feature/people-management-sync
from
feature/current-user-capabilities
Apr 25, 2016
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
980b137
Adds capabilities field and migration to Blog
oguzkocer 640338a
Fix a small spelling issue in WordPressDB
oguzkocer 418c225
Save capabilities retrieved from the blog
oguzkocer f747c5d
Adds checkCapability method to Blog model
oguzkocer ed6e39f
Introduce Capability enum
oguzkocer 7af94dd
Use list_users capability to decide if the user can see the People row
oguzkocer 419359b
Adds missing toString method to Capability enum which was causing a c…
oguzkocer 68258ec
Adds a helpful comment to checkCapability
oguzkocer 6d0f21a
checkCapability method renamed to hasCapability
oguzkocer 02edafc
Fixed a possible NPE regarding capabilities check for a Blog
oguzkocer 3fa0512
Use getString instead of get on JSONObject for getting capabilities
oguzkocer 46fe846
Better naming for checking capability in MySiteFragment
oguzkocer 5d7fcd3
Handle configuration header visibility depending on Settings & People…
oguzkocer 324074d
Instead of overriding toString, use getLabel method for Capability
oguzkocer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 21 additions & 0 deletions
21
WordPress/src/main/java/org/wordpress/android/models/Capability.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package org.wordpress.android.models; | ||
|
|
||
| /** | ||
| * Used to decide what can the current user do in a particular blog | ||
| * A list of capabilities can be found in: https://codex.wordpress.org/Roles_and_Capabilities#Capabilities | ||
| */ | ||
| public enum Capability { | ||
| EDIT_USERS("edit_users"), // Check if user can change another user's role | ||
| LIST_USERS("list_users"), // Check if user can visit People page | ||
| PROMOTE_USERS("promote_users"); // Check if user can invite another user | ||
|
|
||
| private final String label; | ||
|
|
||
| Capability(String label) { | ||
| this.label = label; | ||
| } | ||
|
|
||
| public String getLabel() { | ||
| return label; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 think it's pretty safe to assume that we will use
checkCapability()quite often and so it makes sense to optimise this and avoid instantiating a new JSONObject (rather costly operation that includes traversing and tokenizing the string) every time we need to check a capability. Have you considered (and possibly dismissed?) the option of having theBlogobject have thecapabilitiesfield be a JSONObject directly?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 haven't considered before, but thinking about it now, I don't think I agree that we're going to check the "capabilities" as often as you think. Aside from People Management, I think we're only going to check
list_usersfor once. Also, we tend to read the blog from db a lot and if we stored it asJSONObjectwe'd do that expensive operation even when it's not used.I think a middle ground solution here would be to just cache the capabilities as a
JSONObjectwhen it's first requested, so we avoid the situations where we query the db for a network request or something (rather than building the UI). What do you think?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 was mostly referring to the "rest" of the capabilities (like
edit_usersand others that are very likely we will employ soon) but I agree that all in all we're probably gonna be loading the model more often than we'll need the capabilities checked.I did consider the option of caching the object on first use but opted against suggesting it because of the ugliness of the code around how to determine whether the object is cached or not (checking for null won't be enough since the json can be malformed, leading to null anyway). It would need a boolean instance variable to flag the fact we tried to load the json and that was the bit I considered ugly and dropped the suggestion. That was a thought experiment though so I could be wrong and this can end up be nice!
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 see what you mean. What I'd like to do in this case is, keep this as is for now since it's not getting merged to
developjust yet. Once we start using other capabilities, we can re-visit this to make a more conscious decision. Depending on the trade-offs we can go with either of these 3 solutions.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 agree with @hypest here, and I'd go even deeper and store a custom POJO - it actually depends if these capabilities can be changed, can we add new capabilites, can we do something with these new capabilities within the app (like display them even if they won't be used by the app itself).
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.
The capabilities for now is just for checking them for certain People Management actions. I don't think there will be anyway to update capabilities without updating someone's role. They are tied to each other.
I'll make the change suggested by @hypest in another PR before this gets merged into
develop. Thanks for the feedback @maxme!