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

[ZEPPELIN-1736] Introduce trash & enable removing folder #1730

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@tae-jun
Contributor

tae-jun commented Dec 6, 2016

What is this PR for?

image
This PR introduces the trash! (which is essential, I think ^^;)

I added basic operations of the trash:

Added operations of trash

  • Move a folder to the trash
  • Move a note to the trash
  • Restore a folder
  • Restore a note
  • Remove a folder in the trash for good
  • Remove a note in the trash for good
  • Empty the trash
  • Restore all in the trash
  • Move a note to the trash from the notebook view
  • Remove a note to the trash for good from the notebook view
  • If there is a folder that has the same name, suffix current data rather than just merging(please see screenshot below!)

What type of PR is it?

[Feature]

Future work

Maybe it would be better if notebook view notices that a note is in the trash.

What is the Jira issue?

ZEPPELIN-1736

How should this be tested?

  • Build this PR
  • Move some folders or notes to the trash
  • Restore them
  • Empty the trash
  • Restore all notes in the trash
  • Remove folders or notes in the trash
  • Remove a folder which has the same folder name in the trash

Screenshots (if appropriate)

Move, restore, remove a note from the main page

note_from_main

Move, restore, remove a folder from the main page

folder_test

Move, remove a note from the notebook view

noteview_test

Restore, empty the trash

restore_empty_trash

Suffix current date if the folder exists in the trash

same_folder_name_test

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 closed this Dec 6, 2016

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

@1ambda

This comment has been minimized.

Contributor

1ambda commented Dec 6, 2016

Let me review and give some feedbacks :) Thanks for implementing the useful feature!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 6, 2016

Thanks, @1ambda 😄

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 6, 2016

image
It failed only at Selenium test twice. I guess it's because I changed the action of remove note button of notebook view to moving to trash.

But I have no idea where to fix! 😢 Please help!

@marchpig

It is a great feature! 👍
I've tested it and found some points that should be considered.

  • Korean notenames are located at the bottom of Trash folder.
  • I think that displaying '~Trash' concatenated notename is a little bit unnatural.
    It would be better if user cannot see '~Trash' prefix and cannot get a chance to restore note by just removing ~Trash prefix.
  • Why don't you change a tooltip text 'for good' to 'permanently'?
  • How about removing 'removed at xxxx-xx-xx' postfix from the folder name when it is restored?
@@ -32,13 +33,24 @@
</i>
</a>
<a style="text-decoration: none;">
<i style="font-size: 13px; margin-left: 2px; cursor: pointer; text-decoration: none;"
<!-- if note is not in trash -->
<i ng-if="!node.isTrash" style="font-size: 13px; margin-left: 2px; cursor: pointer; text-decoration: none;"

This comment has been minimized.

@marchpig

marchpig Dec 6, 2016

Contributor

How about creating a class for this style into "home.css" file?
Same style properties are being used in this file several times.

@@ -126,11 +126,20 @@
<!-- put the delete action by itself for your protection -->
<span class="labelBtn" style="vertical-align:middle; display:inline-block;">
<button type="button"
<button ng-if="note.name.split('/')[0] === '~Trash'"

This comment has been minimized.

@marchpig

marchpig Dec 6, 2016

Contributor

How about creating a function for this comparison into a "controller.js"?
And it would be nice to make a const value for ~Trash!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 6, 2016

@marchpig Thanks for your prompt and high-quality review!

You made really valid points. I agree with those at all.

I chose '~' as prefix because of sort order, but I didn't consider Korean. Thanks!

I will fix what you mentioned when I have some time and ping you again 😄
Thanks again! 👍

@@ -894,6 +930,122 @@ private void removeNote(NotebookSocket conn, HashSet<String> userAndRoles,
broadcastNoteList(subject, userAndRoles);
}
private void removeFolder(NotebookSocket conn, HashSet<String> userAndRoles,

This comment has been minimized.

@cuspymd

cuspymd Dec 6, 2016

Contributor

Is 'Set' interface better than 'HashSet' class as the argument type ?

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 6, 2016

Thanks for the review @cuspymd! I think so, but I used HashSet because of consistency. Other methods used HashSet.

I think if we want to change to 'Set', wouldn't it be better to open new refactoring PR that change all other 'HashSet' to 'Set' for consistency? 😄

What do you think?

@cuspymd

This comment has been minimized.

Contributor

cuspymd commented Dec 7, 2016

@tae-jun Yes. Good Idea~!!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 7, 2016

Thanks @cuspymd 😄

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 8, 2016

@marchpig Sorry for my late response! I changed what you addressed except one thing.

Korean note order

image

Remove date string when restore

remove_date

The one thing is about hiding ~Trash prefix. I definitely agree with you but I think it's not good to make a PR huge. So if you think that feature is essential, I will include it to this PR. But if it's not, I would like to leave that as a future work :)

Actually, I planned to add features related to the trash on notebook view in the future. There are many features need to be implemented!

  • Hide ~Trash prefix
  • Notice a note is in the trash
  • Disable edit note in the trash (I'm not sure that this is needed, maybe limiting user's behavior)
  • Disable renaming note in the trash (not sure as well)

The first and second features are needed in my opinion, so I would like to handle those on next PR!

Please tell me your opinion.
Thanks for the review again. It looks better than before thanks to your comments 😄

@marchpig

This comment has been minimized.

Contributor

marchpig commented Dec 9, 2016

@tae-jun It works well! Thank you very much for accepting my opinions. 😃
I think that handling ~Trash prefix in the future is fine to me because it is not a bug but just my opinion :)

However, I found a minor problem.
When a folder name has a trailing space, it is not moved to trash. For example, "My folder /My note".
I think that you can handle this corner case in the future work!

Thanks again for considering my opinions. 👍

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 11, 2016

Thanks for your careful review @marchpig 😄 I fixed the bug you found!

It turns out that I didn't normalize folder ID when creating a folder. It could be a serious bug if you didn't find it.

Thanks 👍

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

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

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

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

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 11, 2016

CI is green! 😄

I changed Selenium test codes affected by my PR and it works now.

Failed reason

remove note button changed to move to trash but test code still wanted to click remove note. So I made it click move to trash button and now it works :)

Please review!
Thanks

@Leemoonsoo

This comment has been minimized.

Member

Leemoonsoo commented Dec 13, 2016

@tae-jun thanks for the great contribution!
LGTM and merge to master if there're no further discussions.

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 14, 2016

Thanks @Leemoonsoo 😄

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Dec 14, 2016

@tae-jun Nice feature indeed!
Most of cases are working like a charm.
But as @marchpig said above,
screen shot 2016-12-14 at 12 28 46 pm
is this intended result? I removed the note1 and note2 separately, and the time stamped only for latter one: NoteDirB/note2.

And another really minor thing is
screen shot 2016-12-14 at 12 41 20 pm

As you can see there are two trash can icons and their sizes are different now. Hmm.. so I would suggest you to use another icon for Empty trash(not sure which one will be proper for this..kk) since the trash can icon is already used for Move note to trash / Move folder to trash / Remove note permanently / Remove folder permanently.

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 14, 2016

@AhyoungRyu Always thanks for your nice review 😄

Yes, it's intended. A time stamp is generated only if a folder already exists in the trash. But it's not generated when moving a note to the trash.

I made this feature because of a use case like this:

A use case needs time stamp

  1. A user moves "NoteDirA" folder to the trash
  2. And the user create a folder which has the same name ("NoteDirA")
  3. After a month from then, the user forgot the existence of the folder already exists in the trash, and moves the folder to the trash which created later
  4. But it was a mistake and the user wants to restore the folder created later

Necessity of the time stamp for the use case above

If there is no time stamp:
The folder created later will be merged into old one and the folder "NoteDirA" will be messy.

If there is a time stamp:

The folder created later will be moved to the trash with a time stamp postfix. There will be two folders which are "NoteDirA" (old one) and "NoteDirA removed at 2016-12-14 xx:xx:xx" (new one). In this case, the user can restore the folder successfully.

This is why I added time stamp! :-) Please tell me if there is any other idea.

Remove permanently icon candidates (?)

I agree with you so I searched Font Awesome and found some candidates for the icon. I think not only empty trash but also Remove note permanently and Remove folder permanently should be replaced together.

image
image
image
image

The icons above are the candidates I found, but please let me know if you have a better one!

Which icon do you prefer? 😄

Thanks for the review again 👍

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Dec 15, 2016

@tae-jun Appreciate for your precise explanation and picking proper candidates as well :) Tested again to check the time stamp, it makes sense. Then how about attaching the time stamp for every cases to keep consistency? Currently the firstly removed one doesn't show the stamp. Surely It's just my opinion.
And regarding the icon, I couldn't find the better icon that has meaning "Clear" either. So you can use anything that you suggested!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 17, 2016

I changed empty trash, remove folder permanently, and remove note permanently icons to X. I'm not sure this is the best icon for it ^^; but we can change later when we find a suitable icon.

Empty trash icon:

image

Remove folder permanently icon:

image

Remove note permanently icon:

image

And I've been thinking about attaching the time stamp to all, but there are some reasons I worry about:

  • It makes folder name too long. It's not good to see. So, I also removed infix removed at.
  • MacOS also only attaches the time stamp to duplicated folder names. So, only attaching the time stamp to duplicated ones is more familiar to users, I think.

Now, a duplicated folder name looks like this:
image

What do you think @AhyoungRyu?
Thanks for the review 😄

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

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

@1ambda

This comment has been minimized.

Contributor

1ambda commented Dec 19, 2016

@tae-jun

Thanks for good feature :) It works well as described and here are two things to consider.

  1. Trash is represented in ~Trash in the Navbar.

screen shot 2016-12-19 at 1 48 36 pm

  1. Do we really need rename, clear output button on notes in Trash? IMO, we need only 2 options. remove or restore for those notes.

screen shot 2016-12-19 at 1 47 39 pm

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Dec 20, 2016

@tae-jun Thanks for your precise explanation. Yeah it makes sense. And tested again and it looks good for what we talked about :)

It'll be nice to be merged after addressing @1ambda's feedback as well!

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 22, 2016

@AhyoungRyu Always thanks for the review :) Also thanks for your understanding.

@1ambda Thanks for the review! I appreciate it and sorry for my late response.
That was a really nice point. I fixed it as you see below:

image

I tested on Chrome and Firefox. Please check!

As to the second point that you mentioned, I thought a lot of about it as well :)
My opinion is same with you, but I think maybe other users can feel limited.
So I would like to make it configurable as three levels on other PR:

  1. super strict: not even open note
  2. read only: only can open note and read it
  3. unlimited access: able to rename, read, write, and run note

Would you mind if I do that? Please let me know 😄
Thanks for the review again.

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Dec 31, 2016

Thanks @AhyoungRyu 😄

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Jan 1, 2017

@tae-jun Can you try to rebase from master and push again? I guess it would be help to make CI all green. Then I'll happy to merge this one!

var notes = {
root: {children: []},
flatList: [],
flatFolderMap: {},
setNotes: function(notesList) {
// a flat list to boost searching
notes.flatList = angular.copy(notesList);
notes.flatList = _.map(angular.copy(notesList), (note) => {

This comment has been minimized.

@felizbear

felizbear Jan 1, 2017

Contributor

you don't need to copy here. map does not mutate original array

This comment has been minimized.

@tae-jun

tae-jun Jan 1, 2017

Contributor

@felizbear Thanks! You are right! 👍 I will push a commit soon

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Jan 1, 2017

@tae-jun I tried to run npm run test under zeppelin-web in my local, but it failed with below error.

PhantomJS 2.1.1 (Mac OS X 0.0.0) Factory: NoteList should generate both flat list and folder-based list properly FAILED
	TypeError: undefined is not an object (evaluating 'note.name.split') in src/index.js (line 8407)
	src/index.js:8407:36
	arrayMap@bower_components/lodash/lodash.js:1431:33
	map@bower_components/lodash/lodash.js:6693:18
	setNotes@src/index.js:8406:32
	test/spec/factory/noteList.js:78:23
PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 69 of 71 (1 FAILED) (0 secs / 0.4LOG: function (columns, rows, comment) { ... }
PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 69 of 71 (1 FAILED) (0 secs / 0.4PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 70 of 71 (1 FAILED) (0 secs / 0.4LOG: function (columns, rows, comment) { ... }
PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 70 of 71 (1 FAILED) (0 secs / 0.4PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 71 of 71 (1 FAILED) (0 secs / 0.4PhantomJS 2.1.1 (Mac OS X 0.0.0): Executed 71 of 71 (1 FAILED) (0.334 secs / 0.424 secs)

Can you take a look again?

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Jan 1, 2017

@AhyoungRyu Thanks!!! I fixed the test error thanks to you 👍

I guess CI will be green this time 😄

@AhyoungRyu

This comment has been minimized.

Contributor

AhyoungRyu commented Jan 1, 2017

1 build failed with below error, but it's irrelevant to this change.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test (default-test) on project zeppelin-pig: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test failed: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /home/travis/build/apache/zeppelin/pig && /usr/lib/jvm/java-7-oracle/jre/bin/java -Xmx2g -Xms1g -Dfile.encoding=UTF-8 -jar /home/travis/build/apache/zeppelin/pig/target/surefire/surefirebooter300784690996837886.jar /home/travis/build/apache/zeppelin/pig/target/surefire/surefire6525093842604398164tmp /home/travis/build/apache/zeppelin/pig/target/surefire/surefire_327671988278855013716tmp
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :zeppelin-pig

So let me merge into master in this time. Thanks for your long long patience and effort @tae-jun 🌄

@tae-jun tae-jun closed this Jan 1, 2017

@tae-jun tae-jun reopened this Jan 1, 2017

@tae-jun tae-jun closed this Jan 1, 2017

@tae-jun tae-jun reopened this Jan 1, 2017

@tae-jun

This comment has been minimized.

Contributor

tae-jun commented Jan 1, 2017

@AhyoungRyu Finally, it's green!!! Thanks for your care 😄

@asfgit asfgit closed this in 1980240 Jan 2, 2017

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