You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by soralee <gi...@git.apache.org> on 2017/04/10 16:08:33 UTC
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
GitHub user soralee opened a pull request:
https://github.com/apache/zeppelin/pull/2239
Show dialog if owners field set to be empty on shiro
### What is this PR for?
This PR is for a showing dialog when note's permission set `Readers/Writers` but `Owners` is empty.
Currently, after merged #849, if user set `Owners` is empty, it's automatically set as the current user.
So, It would be better to show a dialog before `Owners` is fill with the current user because user might think it is not intended.
Plus, if a user only set `Readers` role, all field(`Writers` and `Owners`) role is filled as the current user like below. It's not necessary to our because the `Owner` role can read and write note.
### What type of PR is it?
[ Improvement ]
### What is the Jira issue?
* [ZEPPELIN-2206](https://issues.apache.org/jira/browse/ZEPPELIN-2206)
### How should this be tested?
1. Turn on shiro.
2. Set permission `Writers` and `Readers` except `Owner` role.
3. Check the dialog appears
### Screenshots (if appropriate)
[Before]
* Set `Readers` and `Writers` except `Owners`
* Set only 'Writers`
* set only `Readers`; this screen is attached above.
[After]
* Set `Readers` and `Writers` except `Owners` ( Set only `Writers`; this result is same)
* Set only 'Readers`
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soralee/zeppelin ZEPPELIN-2206_auto_shiro
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2239.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 #2239
----
commit 3197647131a78e01f7d358145ed80ff9d7c4e188
Author: soralee <so...@zepl.com>
Date: 2017-04-10T09:54:25Z
show dialog if owners field is empty
add dialog for insufficient privileges
remove dialog for insufficient privileges
commit b0ec2ced4cc9e612889b93507e81f91f096c5df7
Author: soralee <so...@zepl.com>
Date: 2017-04-10T16:06:41Z
remove annotaion
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@felixcheung Thanks for detail suggestion! That's sounds good. Let me update as each comment.
@1ambda Sure, I agree! I'll remove `!!!`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Hi, there.
CI became green now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@soralee it's disappearance is based on multi-user logic and it's correct since if note have `owners`, `readers`, `writers` as `user1` it doesn't have anything to do in `admin`'s list. That's why I was telling that not sure that this PR makes sense, because of intervening into that flow. From another point when creating note, it's `owner` is automatically assigned as current user without asking as well, so I think having already existing logic and assigning current user as owner (unless having admin feature) is fine, and we don't need much changes on it for now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@khalidhuseynov Thanks for opinion! Yeah, I see. This PR is for showing a dialog when (already existing backend logic) that note's permission set Readers/Writers but Owners is empty. I think I would better to focus the purpose!
So, next step is that I'd like to update this PR as @felixcheung addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
I modified dialog title and `OK` button.
I've checked the `whitespace` and the `$scope.permissions.owners` array don't insert `whitespace` originally. But for stability, I added `if statement`.
@felixcheung, @1ambda could you guys review this again, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Merging into master if no more discussion
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@felixcheung Thanks for review and useful comments!
I initially think something wrong. Let me modify this part again!
Thanks a lots!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Ping!
@felixcheung @khalidhuseynov @1ambda, could you guys confirm again, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@felixcheung, regarding your comment
> hey sorry, so in your screen shot here #2239 (comment)
if cancel when owner is empty it resets also writer/reader, is that a bit odd?
that screenshot means it is reverted with old value if user click `cancel` button.
So, it's describing that
1. original data is only `owners: admin`,
2. and change data of roles
3. and then click `cancel` button.
4. make sure data is reverted by original data (`owners: admin`)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@felixcheung Thanks for review!
Yeah, you're right. It would be better if we also check on the REST API side stably.
So, let me create a JIRA ticket and revert code in `REST API`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2239
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2239
hey sorry, so in your screen shot here https://github.com/apache/zeppelin/pull/2239#issuecomment-297239470
if cancel when owner is empty it resets also writer/reader, is that a bit odd?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
reopen for CI
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
Posted by soralee <gi...@git.apache.org>.
Github user soralee closed the pull request at:
https://github.com/apache/zeppelin/pull/2239
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@khalidhuseynov Thanks for comments!
You mean, this can be occurred another problem like the following steps.
1. A user who has admin permissions sets `readers` and `writers` and empty `owners` roles in `Note permissions'.
2. The user clicks 'Save' button in `Note permissions`
3. The user clicks `Cancel button` in the new dialog(`Setting Owners Permissions`) which I made.
3. then, the default `owners` value is disappeared.
4. and, the user add `user1` to `owners` role and click 'Save' button.
As my understanding your comment, this 3 and 4 steps can be a problem. right?
@khalidhuseynov, Could you check what I understood correctly, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by khalidhuseynov <gi...@git.apache.org>.
Github user khalidhuseynov commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@soralee yeah the steps are correct. so if you follow those steps and click `Save`, say for `Note A`, then `Note A` will disappear from the list of your notes, and then you will not have permissions to view `Note A`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@1ambda Thanks for information always :+1:
I already worked to rebase and run build successfully, but now, I'm checking the `white space` problem.
Let me updated ASAP!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Can we remove `!!!` from dialog title? it's meaningless and noisy. although it's not @soralee's change, i think we can handle it in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
Posted by soralee <gi...@git.apache.org>.
GitHub user soralee reopened a pull request:
https://github.com/apache/zeppelin/pull/2239
Show dialog if owners field set to be empty on shiro
### What is this PR for?
This PR is for a showing dialog when note's permission set `Readers/Writers` but `Owners` is empty.
Currently, after merged #849, if user set `Owners` is empty, it's automatically set as the current user.
So, It would be better to show a dialog before `Owners` is fill with the current user because user might think it is not intended.
Plus, if a user only set `Readers` role, all field(`Writers` and `Owners`) role is filled as the current user like below. It's not necessary to fill because the `Owner` role can read and write note.
### What type of PR is it?
[ Improvement ]
### What is the Jira issue?
* [ZEPPELIN-2206; Be added a writer account when adding a reader account on Shiro](https://issues.apache.org/jira/browse/ZEPPELIN-2206)
* [ZEPPELIN-2418; show dialog if user set to the empty owner permission](https://issues.apache.org/jira/browse/ZEPPELIN-2418)
### How should this be tested?
1. Turn on shiro.
2. Set permission `Writers` and `Readers` except `Owner` role.
3. Check the dialog appears
### Screenshots (if appropriate)
[Before]
* Set `Readers` and `Writers` except `Owners`
* Set only 'Writers`
* set only `Readers`; this screen is attached above.
[After]
* Set `Readers` and `Writers` except `Owners` ( Set only `Writers`; this result is same)
* Set only 'Readers`
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soralee/zeppelin ZEPPELIN-2206_auto_shiro
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2239.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 #2239
----
commit 6168166fc39af5cf5df37a17ec17a488d145b531
Author: soralee <so...@zepl.com>
Date: 2017-04-10T09:54:25Z
check empty and anonymous and revert config when cancel click
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@soralee Since conflicts are about lint errors, you can simply rebase by
1. keeping your change and run `rebase --continue`
2. then, execute `eslint src --fix` in `zeppelin-web` dir.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Hi, @felixcheung :slightly_smiling_face:
I added JIRA url about this improvement in this PR description and I reverted some code in `notebook Rest API` side except that, if only setting `Readers` role, all field(Writers and Owners) role is filled as the current user. Because the `Owners` role can already read and write note, we don't need to fill in `Writers` role.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@felixcheung I updated as your advice!
@khalidhuseynov I added a logic which is reverted with old value if user cancel button like above `4 step`. How about this?
> 4. then, the default owners value is disappeared.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
Posted by soralee <gi...@git.apache.org>.
GitHub user soralee reopened a pull request:
https://github.com/apache/zeppelin/pull/2239
Show dialog if owners field set to be empty on shiro
### What is this PR for?
This PR is for a showing dialog when note's permission set `Readers/Writers` but `Owners` is empty.
Currently, after merged #849, if user set `Owners` is empty, it's automatically set as the current user.
So, It would be better to show a dialog before `Owners` is fill with the current user because user might think it is not intended.
Plus, if a user only set `Readers` role, all field(`Writers` and `Owners`) role is filled as the current user like below. It's not necessary to our because the `Owner` role can read and write note.
### What type of PR is it?
[ Improvement ]
### What is the Jira issue?
* [ZEPPELIN-2206; Be added a writer account when adding a reader account on Shiro](https://issues.apache.org/jira/browse/ZEPPELIN-2206)
* [ZEPPELIN-2418; show dialog if user set to the empty owner permission](https://issues.apache.org/jira/browse/ZEPPELIN-2418)
### How should this be tested?
1. Turn on shiro.
2. Set permission `Writers` and `Readers` except `Owner` role.
3. Check the dialog appears
### Screenshots (if appropriate)
[Before]
* Set `Readers` and `Writers` except `Owners`
* Set only 'Writers`
* set only `Readers`; this screen is attached above.
[After]
* Set `Readers` and `Writers` except `Owners` ( Set only `Writers`; this result is same)
* Set only 'Readers`
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/soralee/zeppelin ZEPPELIN-2206_auto_shiro
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2239.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 #2239
----
commit af4ff29be8a20f597670e7610305d5956243ac2e
Author: soralee <so...@zepl.com>
Date: 2017-04-10T09:54:25Z
rebase master and revert notebookRestApi
commit 047029944014c606f25df6b0c3b4fe82a394caca
Author: soralee <so...@zepl.com>
Date: 2017-04-20T05:05:07Z
modify title of dialogs
commit 3913e00f07a3bfeae802043dcbb93857d6c6ab14
Author: soralee <so...@zepl.com>
Date: 2017-04-20T06:05:59Z
rebase notebook.controller
commit 783fd948a4dc87d07fb581a957fb9933fb504542
Author: soralee <so...@zepl.com>
Date: 2017-04-20T08:34:15Z
add to check whitespace
commit 5ed8b26c47859a01286f1dbf40b11008916a06dc
Author: soralee <so...@zepl.com>
Date: 2017-04-20T09:03:51Z
add to check whitespace
commit ea274830bbf0dd81389c99f20630565f3feefec4
Author: soralee <so...@zepl.com>
Date: 2017-04-20T09:24:10Z
add if statement for checking whitespace
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Ping!
And this CI fail is irrelevant this PR.
```
Tests in error:
SecurityRestApiTest.testGetUserList:69 » JsonSyntax java.lang.IllegalStateExce...
SecurityRestApiTest.testTicket:55 » JsonSyntax java.lang.IllegalStateException...
Tests run: 58, Failures: 5, Errors: 2, Skipped: 1
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2239: Show dialog if owners field set to be empty on ...
Posted by soralee <gi...@git.apache.org>.
Github user soralee closed the pull request at:
https://github.com/apache/zeppelin/pull/2239
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
Ready to review!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2239: Show dialog if owners field set to be empty on shiro
Posted by soralee <gi...@git.apache.org>.
Github user soralee commented on the issue:
https://github.com/apache/zeppelin/pull/2239
@khalidhuseynov thanks for checking it really!
You're right! I think so too! this should be considered.
Furthermore, I checked above steps which I addressed, I saw that zeppelin run strange operation.
Let me improve it so that it does not disappear.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---