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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZEPPELIN-1629] Enable renaming folder from the main page #1630

Closed
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tae-jun
Contributor

tae-jun commented Nov 14, 2016

What is this PR for?

This PR will enable renaming folder from the main page.

I wrote some codes. Hope this PR is the start of folder-based features. (e.g folder authorization, folder focused view)

Please check when you have some time!
Thanks 馃槃

What type of PR is it?

[Feature]

Todos

  • - Fix ConcurrentModificationException of FolderView

What is the Jira issue?

ZEPPELIN-1629

How should this be tested?

Change folder names!

Screenshots (if appropriate)

rename_folder

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@tae-jun tae-jun changed the title from [ZEPPELIN-1629] Enable renaming folder from the main page to [ZEPPELIN-1629] [WIP] Enable renaming folder from the main page Nov 15, 2016

@tae-jun tae-jun changed the title from [ZEPPELIN-1629] [WIP] Enable renaming folder from the main page to [WIP] [ZEPPELIN-1629] Enable renaming folder from the main page Nov 15, 2016

@tae-jun tae-jun changed the title from [WIP] [ZEPPELIN-1629] Enable renaming folder from the main page to [ZEPPELIN-1629] Enable renaming folder from the main page Nov 15, 2016

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 15, 2016

Fixed error and CI is green!

Ready for review! 馃槃

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 15, 2016

Awesome. Let me test this. Will ping you again :)

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 18, 2016

@tae-jun Sorry for my late response.
I tested but can't edit the dir name in root dir level. The folder name can be modified only in below level. Please see the below attached gif img.
dirname

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 18, 2016

@AhyoungRyu Thanks for review 馃槃 Don't need to hurry! I will check and ping you :)

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 18, 2016

@AhyoungRyu ping ping~ I tested!

rename_folder_for_rootdir

But it just works fine with me. I tested both running ZeppelinServer from IntelliJ and binary built from mvn clean package -DskipTests

I use macOS 10.12.1 and Chrome. Could you please share your test environment? 馃槃

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 18, 2016

@tae-jun Build it again and tested, but It works well as you intended. It was my bad.
I tested two cases: \w & \wt Shiro. Both are working well.

Probably it's a nitpick,
screen shot 2016-11-18 at 7 00 52 pm

when I try to rename the folder name at home screen, the above msg is shown up. Could you change renameFolder Notebook to rename folder? (I know renameFolder comes from op and notebook is hardcoded, but sounds a bit awkward 銋淿銋)

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 19, 2016

@AhyoungRyu Thanks for your kind review. I appreciate it.

Actually, I didn't test with Shiro ^^; and didn't know what op parameter means. So thanks very much.

I changed renameFolder to rename folder of you addressed and also renameNote to rename.

Now it looks like this:
2016-11-19 6 37 26

2016-11-19 6 31 21

I also included note name which needs permission 馃槃

What do you think?

@tae-jun tae-jun closed this Nov 19, 2016

@tae-jun tae-jun reopened this Nov 19, 2016

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 20, 2016

@tae-jun It works well when I tried to rename folder name. And I think It's good idea to let user know which notes that he don't have permission exactly. Thanks :)
But currently CI is failed with below compilation error. Could you take a look again?

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/travis/build/apache/zeppelin/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java:[774,5] cannot find symbol
  symbol:   class Folder
  location: class org.apache.zeppelin.socket.NotebookServer
[INFO] 1 error
@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 20, 2016

@tae-jun And one more thing, it would be better to keep consistency between Note and Notebook(combination of several notes). I pushed one commit to your branch, could you please take a look tae-jun#1?

It will be shown when user rename the folder(I removed notebook):
screen shot 2016-11-20 at 10 25 22 am

And then user rename the note:
screen shot 2016-11-20 at 10 25 05 am

And also when user create a note, I changed Example: NoteDir1/Notebook1 -> NoteDir1/Note1
screen shot 2016-11-20 at 10 24 01 am

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 20, 2016

@AhyoungRyu Thanks! I also think notebook is a little bit weird :) I merged your PR thanks.

This change will affect other insufficient privileges error messages as well:

  • Insufficient privileges to read notebook
  • Insufficient privileges to update notebook
  • Insufficient privileges to remove notebook
  • Insufficient privileges to write notebook
  • Insufficient privileges to clear output notebook

I prefer removing notebook. What do you think guys? If anyone has a concern about this, please tell.

And about CI, I suffered from that error when I tried to compile only one project (e.g. zeppelin-server). Since zeppelin-server depends on zeppelin-interpreter and zeppelin-zengine, I had to build two dependencies and install them on local maven repo, which can be done with mvn clean package install -DskipTests. If not, when it tries to compile zeppelin-server, it cannot find symbols of zeppelin-zengine and goes to fail.

However, I'm not sure that CI only recompiles changed files. I will look into it, and tell you if I get some more information.
(CI was green before 馃槶)

@@ -713,7 +713,7 @@ private void renameNote(NotebookSocket conn, HashSet<String> userAndRoles,
NotebookAuthorization notebookAuthorization = notebook.getNotebookAuthorization();
if (!notebookAuthorization.isOwner(noteId, userAndRoles)) {
permissionError(conn, "rename", fromMessage.principal,
permissionError(conn, "rename the note", fromMessage.principal,

This comment has been minimized.

@AhyoungRyu

AhyoungRyu Nov 20, 2016

Contributor

@tae-jun I didn't care about the other operations as you said. It was my bad. I just thought about the renaming.. How about just let this "rename the note" -> "rename" as before, and put note in here?

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 20, 2016

@AhyoungRyu I fixed what you mentioned 馃槃 Please review.

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 20, 2016

I guess CI fails because it only recompiles changed files. (since build time is too short)

image

It should be about 30min.

Is there any way to compile the whole project again? I ran CI on my own repository. It failed on some profiles because of Ignite interpreter. (what is relevance between Ignite interpreter and my PR? 馃槶) But it passed on other profiles whereas every profile failed on this repository.

@tae-jun tae-jun closed this Nov 23, 2016

@tae-jun tae-jun reopened this Nov 23, 2016

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 29, 2016

CI failure is because of me. I will fix it :)

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 29, 2016

@tae-jun Great work! Take your time and feel free to ping me again when you're ready.

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 29, 2016

@AhyoungRyu Yeah! CI is green 馃槃 Please review! Thanks

@soralee

This comment has been minimized.

Contributor

soralee commented Nov 30, 2016

@tae-jun This is Cool! and It works well!
So, I suggest one thing. I think it would be nice to appear popup which is whether user will merge the folders or not before merging folders. In my case, It is familiar to me 馃槃

What do you think?

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 30, 2016

@soralee Thanks for the review! 馃槃 That's a great idea! I will give some effect on the front-end and ping you again. Thanks :)

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 30, 2016

@soralee Hi! I added the feature you suggested. What do you think? Is title or content of warning message appropriate? Thanks for the cool suggestion!

rename_folder_merging_warning

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Nov 30, 2016

@soralee What a nice suggestion! @tae-jun I think it's cool!!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Nov 30, 2016

@AhyoungRyu Always thanks for the review 馃槃

@tae-jun tae-jun closed this Nov 30, 2016

@tae-jun tae-jun reopened this Nov 30, 2016

@soralee

This comment has been minimized.

Contributor

soralee commented Nov 30, 2016

@tae-jun It is very AWESOME 馃憤 and thank you for the accept my suggestion.
Let me check again after CI is green 馃槃

@tae-jun tae-jun closed this Nov 30, 2016

@tae-jun tae-jun reopened this Nov 30, 2016

@tae-jun tae-jun closed this Dec 1, 2016

@tae-jun tae-jun reopened this Dec 1, 2016

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 1, 2016

@AhyoungRyu @soralee Yeah! CI is green!

@Leemoonsoo

This comment has been minimized.

Member

Leemoonsoo commented Dec 2, 2016

Tested and working really nicely. Looks awesome to me!
Thanks @tae-jun for the great contribution!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 2, 2016

Thanks @Leemoonsoo! I appreciate your review 馃槃

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 2, 2016

I've tested more and it was fine most times. But occasionally it doesn't work well. I think it should be tested more! I don't know why for now but I will figure it out and ping when it's ready :)

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 3, 2016

I found out bugs by adding loggers and fixed them!

CI is green and it seems to work perfectly. I think it's good to go now 馃槃

Please when you have some time. Thanks!

@Leemoonsoo

This comment has been minimized.

Member

Leemoonsoo commented Dec 3, 2016

Great work @tae-jun. LGTM!

@Leemoonsoo

This comment has been minimized.

Member

Leemoonsoo commented Dec 4, 2016

Merge to master if there're no more comments

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Dec 4, 2016

Tested and it's working nicely! LGTM 馃憤

@asfgit asfgit closed this in fd7a3f8 Dec 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment