[ZEPPELIN-1142] Zeppelin allows two users to simultaneously edit the notebook permissions #1393
Conversation
| AuthenticationInfo subject = new AuthenticationInfo("user1"); | ||
| Note note = notebook.createNote(subject); | ||
|
|
||
| Notebook notebook = new Notebook( |
There was a problem hiding this comment.
why creating new notebook instance here? can just reused previous one, isn't it
There was a problem hiding this comment.
The notebook instance is different from note where the former holds the collection of the latter.
There was a problem hiding this comment.
yeah that's true, but the question is why you instantiate notebook with new Notebook on line 218 if you already used different instance of it to create note on line 216. you can reuse same instance and just delete instantiation at line 218 , isn't it?
There was a problem hiding this comment.
Yeah that was correct.Sorry for missing them out. I have made the changes and pushed them.
|
@kavinkumarks thanks for improvement and it makes sense indeed to assign owner when creating note. just need to address above minor comments as well as restart CI, some profiles are failing. |
kavinkumarks
force-pushed
the
zeppelin-1142-simultaneous-note-permission-error
branch
from
d794a2a
to
4412692
Compare
|
@khalidhuseynov thanks for the review! I have replied to the comments and the CI build is in progress. |
|
Reopening to trigger the CI build. |
|
Found an build test failure, working on it. |
cases for the relevant scenarios.
prinicipal is not null.
kavinkumarks
force-pushed
the
zeppelin-1142-simultaneous-note-permission-error
branch
from
85af4e6
to
5a45c9e
Compare
|
Fixed the build test failure and the CI build is in progress. |
|
The CI build is green. |
|
LGTM |
|
Thanks @khalidhuseynov ! Can we get this merged? -Kavin |
|
@kavinkumarks i believe some of the committers or pmcs will take a look into this pr before merging |
|
Could someone else have a look at this and merge if everything is okay? Thanks, |
|
Thanks @kavinkumarks for the contribution. Before, owner is not defined when creating note. Especially with code changes such as #1330, user will experience bigger behavior changes. But i think having current user as a owner when creating note is natural behavior that most people can think, even though it brings some UX changes. LGTM and merge if there're no further discussions. |
|
Thanks @Leemoonsoo for reviewing this! Could you please merge this?
|
…notebook permissions ### What is this PR for? This is about storing the owner information on creating a note so when the same user tries to edit the permissions of the note he could do it successfully. ### What type of PR is it? Improvement ### Todos NA ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1142 ### How should this be tested? Check the below cases via the web application or REST API calls and the owner information should be persisted properly. * Create note * Clone note * Import note ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Kavin <kavin.kumar@imaginea.com> Closes apache#1393 from kavinkumarks/zeppelin-1142-simultaneous-note-permission-error and squashes the following commits: 5a45c9e [Kavin] Ensure that the authentication instance is created only when the input prinicipal is not null. 7642f63 [Kavin] Removed the duplicate instance of notebook variable and reused the existing one. e1b8b08 [Kavin] Store owner information on creating a note and added integration test cases for the relevant scenarios.
What is this PR for?
This is about storing the owner information on creating a note so when the same user tries to edit the permissions of the note he could do it successfully.
What type of PR is it?
Improvement
Todos
NA
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1142
How should this be tested?
Check the below cases via the web application or REST API calls and the owner information should be persisted properly.
Screenshots (if appropriate)
Questions: