[ZEPPELIN-209] Folder support for notebooks (based on pr-190 & pr-767)

[johnnyws]

What is this PR for?

Add folder support. When create a new notebook, use '/' to separate folders.

Example: /FolderA/NotebookA

What type of PR is it?

Feature

Todos

• - Should disable clicking the folder in dropdown menu
• - Add instruction about how to add folder in the notebook create menu
• - Sort the notes / dirs by name
• - Move to lodash

What is the Jira issue?

Zeppelin-209

How should this be tested?

Screenshots (if appropriate)

Questions:

• Does the licenses files need update?

NO

• Is there breaking changes for older versions?

NO

• Does this needs documentation?

NO

[johnnyws]

I am bring this back to live again. I made a couple of changes based on the comments of the previous pull request. Now it is ready to review/merge

[heruka-urgyen]

@johnnyws what is your opinion on this comment?

[johnnyws]

@felizbear Thanks for your comments. Please correct me if I am wrong, because I have very limited knowledge in javascript. It seems we can create objects dynamically using syntax of _.{set,get}? This is really cool! Can we easily enumerate & sort all fields of an object in the view templates?

Currently, it seems sorting over lists works naturally well. I didn't write any code for sorting the notebooks, but the folders will appear on top if it starts with "/". I am not sure how does it work, but the sorting behavior is quite consistent.

[heruka-urgyen]

You can map over these objects either using lodash, or doing something like
Object.keys(obj).map(key => obj[key]).sort(...).

[bzz]

@felizbear thank you for a review!
@johnnyws thank you for stepping up and bringing this back to life.

CI is failing on web application build, so may be you should check what ./grunt jshint returns.

[johnnyws]

It seems some recent change breaks the karma unit tests. I saw such failures while running unit tests:

  Error: [$injector:modulerr] Failed to instantiate module zeppelinWebApp due to:
  Error: [$injector:modulerr] Failed to instantiate module as.sortable due to:
  Error: [$injector:nomod] Module 'as.sortable' is not available! You either misspelled the module name or forgot to load it. If registering a module ensure that you specify the dependencies as the second argument.
  http://errors.angularjs.org/1.3.8/$injector/nomod?p0=as.sortable

Any clues?

[heruka-urgyen]

Maybe you should do bower install?

[johnnyws]

Thanks @felizbear, the testing works now.

Lodash looks cool, and I really hope I can make the folder construction code more concise using fp. However, after I tried it and thought a little more, I found it may not be ideal for our case, because a file name can contain any characters, which is not suitable as an object identifier. I felt struggling dealing with characters like ".[{" in file names.

Any suggestions?

[heruka-urgyen]

yes, seems like lodash has a problem setting values with . in them since it uses dot for denoting nested properties. what you can do, is set them as prop["notebook-name-with-dot-and-other-symbols"] = val and then access them _.get. what do you think?

[johnnyws]

@felizbear thought about this, too. There will be one more special case which may make the construction/accessing code more complicated using map-like structures:
/A/B/C
/A/B (a file named B)

We need some extra code to deal with such case. Because currently we allow users to create notes with the same name, I am not sure how simpler using map-like structure can be than using lists.

[heruka-urgyen]

in my opinion, it is more maintainable to forbid duplicate names hence simplify getting / setting mechanism. is duplicates a useful feature?

[johnnyws]

I don't think it's useful, but probably we need another pr to forbid it with name duplication validation. Besides, we need to consider backward compatibility.

I would like to work on it, but I think the folder support will be blocked for a while.

[heruka-urgyen]

Since we don't have folders yet, then with what to consider backward compatibility?
In my opinion, it's more important to implement this feature well than fast, so a separate PR with duplication validation sounds like a good idea.

@bzz what do you think?

[zhongneu]

@felizbear if we forbid folder-file name duplication, shall we also forbid file-file name duplication to make the behavior more consistent? here the backward compatibility issue comes...

If we only consider folder-file name duplication, I don't think a separate PR is necessary

[heruka-urgyen]

I don't see a problem with having same notebook names in different folders; it might be useful actually. Or did I misunderstand you?

[zhongneu]

@felizbear yes, I mean files in the same folder, or the full path. currently, we can create files with the same name:
"this is a file"
"this is a file"

Shall we allow create files like this?
"/this/is/a/file"
"/this/is/a/file"

I think this 2 behavior should be consistent.

[bzz]

@johnnyws rebasing the branch on top of the latest master should make the CI green and get rid of current failure.

Let me think a bit about validation though, I'm a bit confused here as before we were talking about /FolderA/NotebookA and note does not have a notion of file, so I'm not sure what file-name duplication here means.

[heruka-urgyen]

@zhongneu I'm still unclear about your example

"/this/is/a/file"
"/this/is/a/file"

looks like the same file to me; there is no way that two equal paths can lead to different files.

Again, in my opinion, notebook names can have duplicates as long as they are located in different folders. Much like in your computer's filesystem.

[johnnyws]

@felizbear the problem is that we are using notebook name to represent paths, and we allow notebooks with same name. Thus, I think having 2 notebooks with the same name (such as /this/is/a/note) should be valid. Then we actually allow the notebooks in the same folder having the same name. (I am trying to avoid using the term of "file" here, because for file systems, we do not allow files with same name in the same directory, but our case is slightly different from a typical file system).

This issue may make the implementation more complicated if we use map-like data structures, because we need to deal with the case with duplicates. Thus, I think it may be more appropriate to use list-based data structures.

I do agree that the code quality is very important, and I admit that the current logic of folder construction is a little bit messy. I would like to refactor the code once we have an agreement on either to use map-based data structure or list-based data structure.

[johnnyws]

Just add one more comment: if we plan to forbid the users having notebooks with the same name (notebook name, not "file" name), we need to consider about the backward compatibility issue.

I hope these statements can make you clear. Sorry if I introduce any confusion.

[heruka-urgyen]

@johnnyws I understand. The key in map-based implementation should be notebookId because it's unique. That said, I don't want to push you with this approach. If you think that list-based implementation does the job well and is maintainable, then let's keep it. It would benefit from a slight refactoring, in my opinion, just to make the code more readable.

The problem of adding backward compatibility for duplicate names is probably out of scope of this PR.
In my opinion, duplicated names shall not be allowed. I struggle to see any valid use case here. On the front-end, backward compatibility problem could potentially be solved by adding a unique postfix to names (e.g. notebook id) in the model that isn't displayed in the view.

[bzz]

@johnnyws thank you for explanation. I think there is value in this PR not to changing the default behaviour that users already have for the 'root', single-folder case. Although having notebooks with the same name is very confusing in my experience and better be avoided.

Uniq note name per-folded as @felizbear suggests makes sense to me and can be added here or later in a separate PR either as validation or as some composite key of the map.

[johnnyws]

@felizbear did some refactoring, and ready for review. btw, I really enjoyed using lodash
@bzz i did merge several times on this branch. do I need to rebase? or github can crunch the commits together?

[johnnyws]

JIRA created to deal with notebook name duplicates: ZEPPELIN-796

[heruka-urgyen]

@johnnyws looks good. a few minor comments

• creating en empty object and assigning props to it is unconventional in JS; normally, you would create an object together with the props:

// instead of 
 var notes = {};
 notes.root = {
    children : []
 };
 notes.flatList = [];

// better do
var notes = {
  root: {children: []},
  flatList: [],
  setNotes: function() {}
}

• I'd move addNode declaration outside of setNotes to prevent it from being recreated on each step of reduce.

[johnnyws]

Thanks, @felizbear ! I've made changes based on your comments. Passed karma & jshint. Waiting green light from Travis and ready to merge.

[heruka-urgyen]

@johnnyws I think you understand my comment too literally :)
setNotes: function() {/* the function goes here */} should actually implement the function here; defining an empty function and later redefining it is rather useless :)

[johnnyws]

updated. I thought it was some practice of js to use it as some kind of declaration :)

[heruka-urgyen]

I think we can merge it

[felixcheung]

any more comment, @zhongneu, @Leemoonsoo , @bzz?

[Leemoonsoo]

Tested this branch and found some strange behavior.
After creation of new notebook, existing notebooks are displayed twice in the list, until refresh the page.

[johnnyws]

@Leemoonsoo I've fixed the issue and rebased with master

[Leemoonsoo]

@johnnyws Tested and working great. LGTM

[bzz]

Looks great to me!
Merging if there is no more discussions.

[sumitsidana]

Hi @johnnyws,

The folder structure does not seem to be working for me. I have tried using / but it does not seem to work. Also, I have pulled latest changes from repository. Am I missing something?
Please find attached the screen shot of my zeppelin.

Thank you for your help
Sumit