You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by scottyaslan <gi...@git.apache.org> on 2018/02/02 13:43:26 UTC
[GitHub] nifi-registry pull request #99: [NIFIREG-126] adding some polish and testing...
GitHub user scottyaslan opened a pull request:
https://github.com/apache/nifi-registry/pull/99
[NIFIREG-126] adding some polish and testing around deep links
To test this PR:
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/scottyaslan/nifi-registry NIFIREG-126
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-registry/pull/99.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #99
----
commit bbf9d96abc96023bcc2cf0deeb8c31a1c435e850
Author: Scott Aslan <sc...@...>
Date: 2018-02-01T21:38:28Z
[NIFIREG-126] adding some polish and testing around deep links
----
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on the issue:
https://github.com/apache/nifi-registry/pull/99
@kevdoran I have updated this PR with the new message as well as logging info.
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/99
Thanks @moranr and @scottyaslan! Will do a final pass on this
---
[GitHub] nifi-registry pull request #99: [NIFIREG-126] adding some polish and testing...
Posted by moranr <gi...@git.apache.org>.
Github user moranr commented on a diff in the pull request:
https://github.com/apache/nifi-registry/pull/99#discussion_r167249634
--- Diff: nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessPolicyResource.java ---
@@ -174,7 +174,7 @@ public Response getAccessPolicy(
final AccessPolicy accessPolicy = authorizationService.getAccessPolicy(identifier);
if (accessPolicy == null) {
- throw new ResourceNotFoundException("No access policy found with ID + " + identifier);
+ throw new ResourceNotFoundException("The specified access policy does not exist for this bucket.");
--- End diff --
I think that works, we could even add _"...in this registry."_ to end of that. It's consistent with the other messages.
---
[GitHub] nifi-registry pull request #99: [NIFIREG-126] adding some polish and testing...
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on a diff in the pull request:
https://github.com/apache/nifi-registry/pull/99#discussion_r167246736
--- Diff: nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessPolicyResource.java ---
@@ -174,7 +174,7 @@ public Response getAccessPolicy(
final AccessPolicy accessPolicy = authorizationService.getAccessPolicy(identifier);
if (accessPolicy == null) {
- throw new ResourceNotFoundException("No access policy found with ID + " + identifier);
+ throw new ResourceNotFoundException("The specified access policy does not exist for this bucket.");
--- End diff --
Good catch....I think we can simply change it to say "The specified policy does not exist."?
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by scottyaslan <gi...@git.apache.org>.
Github user scottyaslan commented on the issue:
https://github.com/apache/nifi-registry/pull/99
@kevdoran I have updated this PR with @moranr suggested messaging.
---
[GitHub] nifi-registry pull request #99: [NIFIREG-126] adding some polish and testing...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-registry/pull/99
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by moranr <gi...@git.apache.org>.
Github user moranr commented on the issue:
https://github.com/apache/nifi-registry/pull/99
@scottyaslan , @kevdoran – redirect behavior looks good. Below are recommendations for the dialog titles and messages.
UNSECURED
_-nifi-registry/explorer/grid-list/buckets/0_
_-nifi-registry/explorer/grid-list/buckets/0/flows/0_
- Title – Bucket Not Found
- Msg – The specified bucket ID does not exist in this registry.
_-nifi-registry/explorer/grid-list/buckets/**existing-bucket-id**/flows/0_
- Title – Flow Not Found
- Msg – The specified flow ID does not exist in this bucket.
_-nifi-registry/administration/users_
- Title – Not Applicable
- Msg – User administration is not configured for this registry.
SECURED
_-nifi-registry/administration/workflow(sidenav:manage/bucket/000000000000)_
- Title – Bucket Not Found
- Msg – The specified bucket ID does not exist in this registry.
_-nifi-registry/administration/users(sidenav:manage/user/000000000000)_
- Title – User Not Found
- Msg – The specified user ID does not exist in this registry.
_-nifi-registry/administration/users(sidenav:manage/group/000000000000)_
- Title – User Group Not Found
- Msg – The specified user group ID does not exist in this registry.
_-nifi-registry/administration/*_ (If user does not have applicable admin privileges)
- Title – Access Denied
- Msg – Please contact your System Administrator.
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/99
Reviewing...
---
[GitHub] nifi-registry issue #99: [NIFIREG-126] adding some polish and testing around...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/99
Thanks! Will review
---
[GitHub] nifi-registry pull request #99: [NIFIREG-126] adding some polish and testing...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi-registry/pull/99#discussion_r167076913
--- Diff: nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessPolicyResource.java ---
@@ -174,7 +174,7 @@ public Response getAccessPolicy(
final AccessPolicy accessPolicy = authorizationService.getAccessPolicy(identifier);
if (accessPolicy == null) {
- throw new ResourceNotFoundException("No access policy found with ID + " + identifier);
+ throw new ResourceNotFoundException("The specified access policy does not exist for this bucket.");
--- End diff --
I'm on board with the spirit of this change: removing cryptic UUIDs from the UX error messages, while still providing enough context to be useful/meaningful to the user. In general these changes look fine.
In this particular case, however, I think the wording of this message assumes too much regarding "... does not exist _for this bucket_". Access policies on the backend are generic to all types of resources, not just buckets. So for instance, one could be attempting to add a user to the access policy for the Proxy Resource, or the Tenants (users and groups) Resource. In such a case, it would be confusing to receive an error message referencing buckets.
I think we need use a more generic error message that is suitable for all cases (REST API or UI).
---