[ZEPPELIN-1745] View revisions in non-editable mode #1775

khalidhuseynov · 2016-12-16T08:30:37Z

What is this PR for?

This is to make view of revisions non-editable

What type of PR is it?

Improvement

Todos

- disable action bar buttons
- disable per paragraph editing actions
- disable hotkeys

What is the Jira issue?

ZEPPELIN-1745

How should this be tested?

initialize GitNotebookRepo by adding

export ZEPPELIN_NOTEBOOK_STORAGE="org.apache.zeppelin.notebook.repo.GitNotebookRepo"

in your conf/zeppelin-env.sh
2. go to any note and create few revisions using commit menu
3. then switch between revisions, during which you shouldn't be able to modify 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

bzz · 2016-12-20T01:56:25Z

@khalidhuseynov Is it WIP?

Then please feel free to add WIP in PR title and ping back, when it's ready for reviews. Otherwise, as for any GUI changes, it would be nice to have screenshots and some docs.

khalidhuseynov · 2016-12-20T07:06:35Z

@bzz it's ready now, also /cc @cloverhearts @1ambda to help review

1ambda · 2016-12-20T07:31:04Z

Glad to help reviewing this PR :)
Let me check and then give you feedbacks :)

cloverhearts · 2016-12-20T10:21:58Z

good feature!

1ambda · 2016-12-21T07:10:01Z

For other reviewers

If you are using git 2.10+, Notebook revision feature will not work. Checkout ZEPPELIN-1737

khalidhuseynov · 2016-12-23T05:08:14Z

@1ambda actually i've tried to reproduce ZEPPELIN-1737 and even with git 2.11.0 wasn't able to do it. also from documentation here only from 2.11.0 --allow-empty commits was introduced, meaning only git versions 2.11+ could be a problem which is the most recent version and can be built from the source only for now. So I think reviewing this PR wouldn't much depend on git version, and ZEPPELIN-1737 will be addressed separately.

1ambda · 2016-12-23T05:56:09Z

FYI - you can install 2.11.0 using brew.

@khalidhuseynov is right, that should be handled in the separate issue. Let's talk about it on ZEPPELIN-1737.

1ambda · 2016-12-23T09:56:29Z

Works well as described. Here are few things to consider.

It would be great if there is a signal (whatever) so let user know that this is committed version of note. It's confusing at first for me. (Assume that someone get the link for a commited note. He might wonder why he can not modify this note)
Enabling and modifying committed notes affects on the HEAD note. What about disabling double click to committed paragraphs (you can check this in screenshot below)

the last one is not directly related with this issue. But It would be good to hide git notebook icons if we are other notebook repo rather than GitNotebookRepo

Thanks for implementing great feature.

khalidhuseynov · 2016-12-27T07:16:27Z

@1ambda thanks for review. answers in order:

It would be great if there is a signal (whatever) so let user know that this is committed version of note. It's confusing at first for me. (Assume that someone get the link for a committed note. He might wonder why he can not modify this note)

yeah that's good point, do you have something in mind regarding it? i was thinking whether it should be more of a mode switch (say different mode added in addition to report mode, etc) or just represent visually (say gray out editor background like when running note), or something else.

Enabling and modifying committed notes affects on the HEAD note. What about disabling double click to committed paragraphs (you can check this in screenshot below)

thanks for testing, addressed under 50e1472. please check it out.

the last one is not directly related with this issue. But It would be good to hide git notebook icons if we are other notebook repo rather than GitNotebookRepo.

actually we have issue regarding this one to have GitNotebookRepo as default repo, in that case we don't need to hide it i believe.

normally it should be done on change of .revisionView however at that time editor isn't loaded yet

cloverhearts · 2016-12-20T10:16:54Z

zeppelin-web/src/app/notebook/paragraph/paragraph-control.html

@@ -23,17 +23,17 @@
  </span>

  <!-- Run / Cancel button -->
-  <span class="icon-control-play" style="cursor:pointer;color:#3071A9" tooltip-placement="top" tooltip="Run this paragraph (Shift+Enter)"
+  <span ng-if="!revisionView" class="icon-control-play" style="cursor:pointer;color:#3071A9" tooltip-placement="top" tooltip="Run this paragraph (Shift+Enter)"


I think iterate condition case to group is better.

<span ng-if="!revisionView" class="icon-control-play" style="cursor:pointer;color:#3071A9" tooltip-placement="top" tooltip="Run this paragraph (Shift+Enter)" ng-click="runParagraph(getEditorValue())" ng-show="paragraph.status!='RUNNING' && paragraph.status!='PENDING' && paragraph.config.enabled"> <span ng-if="!revisionView" class="icon-control-pause" style="cursor:pointer;color:#CD5C5C" tooltip-placement="top" tooltip="Cancel" ng-click="cancelParagraph(paragraph)" ng-show="paragraph.status=='RUNNING' || paragraph.status=='PENDING'"> <span ng-if="!revisionView" class="{{paragraph.config.editorHide ? 'icon-size-fullscreen' : 'icon-size-actual'}}" style="cursor:pointer;" tooltip-placement="top" tooltip="{{(paragraph.config.editorHide ? 'Show' : 'Hide') + ' editor'}}" ng-click="toggleEditor(paragraph)"> <span ng-if="!revisionView" class="{{paragraph.config.tableHide ? 'icon-notebook' : 'icon-book-open'}}" style="cursor:pointer;" tooltip-placement="top" tooltip="{{(paragraph.config.tableHide ? 'Show' : 'Hide') + ' output'}}" ng-click="toggleOutput(paragraph)"> 

better

 <span class="icon-control-play" style="cursor:pointer;color:#3071A9" tooltip-placement="top" tooltip="Run this paragraph (Shift+Enter)" ng-click="runParagraph(getEditorValue())" ng-show="paragraph.status!='RUNNING' && paragraph.status!='PENDING' && paragraph.config.enabled"> <span class="icon-control-pause" style="cursor:pointer;color:#CD5C5C" tooltip-placement="top" tooltip="Cancel" ng-click="cancelParagraph(paragraph)" ng-show="paragraph.status=='RUNNING' || paragraph.status=='PENDING'"> <span class="{{paragraph.config.editorHide ? 'icon-size-fullscreen' : 'icon-size-actual'}}" style="cursor:pointer;" tooltip-placement="top" tooltip="{{(paragraph.config.editorHide ? 'Show' : 'Hide') + ' editor'}}" ng-click="toggleEditor(paragraph)"> <span class="{{paragraph.config.tableHide ? 'icon-notebook' : 'icon-book-open'}}" style="cursor:pointer;" tooltip-placement="top" tooltip="{{(paragraph.config.tableHide ? 'Show' : 'Hide') + ' output'}}" ng-click="toggleOutput(paragraph)">

cloverhearts · 2016-12-20T10:20:22Z

zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js

+    };
+
+    $scope.$watch($scope.getEditor, function(newValue, oldValue) {
+      if (newValue === null) {


Newvalue Sometimes value is undefined.
In js, both undefined and null should be checked.
So I think this is better.
` If (newvalue) // check to undefined and null `

was failing to build, problem with removing console messages for prod

khalidhuseynov · 2016-12-30T06:28:37Z

@Leemoonsoo @cloverhearts @1ambda thanks for the review! I addressed all your comments and updated the screenshot above. Please check it out and let me know what you think.

Also CI logs seems to be failing only in selenium profile with flaky test from ZEPPELIN-1836.

AhyoungRyu · 2016-12-30T07:55:44Z

@khalidhuseynov How about disable "Interpreter binding" & "Note permission" buttons as well? Currently I can change the bound interpreters and note permission for the readonly-mode note.

khalidhuseynov · 2016-12-30T10:55:23Z

@AhyoungRyu actually i thought about it and couldn't find a reason why not let user change "Interpreter bindings" and "Note permissions" even in read-only mode, since they're still not able to run or edit. Do you think it's not common sense and confusing? Let me know what you think and we can change that easily. I wasn't also sure about it

AhyoungRyu · 2016-12-30T15:39:20Z

@khalidhuseynov

since they're still not able to run or edit

Yes you're right. I just thought it's not common sense as you told. Even though the user can't run or edit anyway, but he can do sth in interpreter binding page and note permission setting page. That's why I suggested to block those features by disabling the icon buttons.

khalidhuseynov · 2016-12-31T05:45:40Z

@AhyoungRyu I think it makes sense and addressed your feedback. please check it out and let me know if it's fine.

AhyoungRyu · 2016-12-31T07:02:56Z

@khalidhuseynov Thanks for your consideration! Tested again and it works well as we said :)
And there is one minor thing in lookandfeel setting menu. Is this intended behavior?

Head

Read-only mode

khalidhuseynov · 2017-01-01T21:13:27Z

@AhyoungRyu thanks for review, it wasn't intended behaviour actually, fixed in bbaf286

AhyoungRyu · 2017-01-02T07:46:46Z

Tested again and now I can see the text in the looknfeel dropdown menu of non-editable mode note. LGTM

khalidhuseynov · 2017-01-03T07:11:17Z

merged master and finally CI is green. @Leemoonsoo ready for final check

Leemoonsoo · 2017-01-03T17:11:42Z

Tested and LGTM. Thanks @khalidhuseynov for the improvement.
Merge to master if there're no more discussions.

khalidhuseynov closed this Dec 23, 2016

khalidhuseynov reopened this Dec 23, 2016

khalidhuseynov force-pushed the feat/revision-view branch from 50e1472 to e9d3ceb Compare December 27, 2016 09:37

khalidhuseynov added 18 commits December 27, 2016 18:38

change var name revisionDisabled -> revisionView

4c83895

disable run all

fa1397d

disable clear output

8e97800

disable clone note

46238e8

disable export note

50d40ad

disable revision commit

5d9f877

disable remove note

ab8ab4d

disable scheduler

711c0b3

disable + new paragraph

7d115b8

don't show paragraph settings menu

19cacbd

don't show run/cancel on paragraph

c55de95

make paragraph title non-editable

00fb556

disable editor editing

9b85e05

normally it should be done on change of .revisionView however at that time editor isn't loaded yet

disable hotkeys

7751b69

disable editing note name

81346e9

disable show/hide code in note action-bar

0946377

disable show/hide output in note action bar

46d67af

don't show paragraph level hide editor button

1eea8ce

gray out editor on revision view

4bfe560

cloverhearts reviewed Dec 29, 2016

View reviewed changes

khalidhuseynov force-pushed the feat/revision-view branch from d3fface to 06e2977 Compare December 29, 2016 15:08

khalidhuseynov added 6 commits December 29, 2016 07:08

add group span for ng-if

2823142

check newValue for undefined

06e2977

render result change without commit

1a5a6a7

Merge branch 'master' into feat/revision-view

fa05bdb

disable personalized mode button

84d2c5a

remove escaped single quote inside console

843abf4

was failing to build, problem with removing console messages for prod

khalidhuseynov mentioned this pull request Dec 30, 2016

[ZEPPELIN-1850] Remove strip-loader (webpack) #1823

Closed

khalidhuseynov added 2 commits December 30, 2016 14:05

dont show bingings, perms, hotkeys

b3a78c7

enable report view without save

efe09c0

initialize looknfeel

bbaf286

khalidhuseynov force-pushed the feat/revision-view branch from 2672162 to 1439da3 Compare January 3, 2017 04:22

khalidhuseynov closed this Jan 3, 2017

khalidhuseynov reopened this Jan 3, 2017

khalidhuseynov force-pushed the feat/revision-view branch 2 times, most recently from 6b8e1f6 to 59a92ea Compare January 3, 2017 06:14

Merge branch 'master' into feat/revision-view

59a92ea

asfgit closed this in d9a11a9 Jan 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-1745] View revisions in non-editable mode #1775

[ZEPPELIN-1745] View revisions in non-editable mode #1775

khalidhuseynov commented Dec 16, 2016 •

edited

bzz commented Dec 20, 2016 •

edited

khalidhuseynov commented Dec 20, 2016

1ambda commented Dec 20, 2016

cloverhearts commented Dec 20, 2016

1ambda commented Dec 21, 2016

khalidhuseynov commented Dec 23, 2016

1ambda commented Dec 23, 2016

1ambda commented Dec 23, 2016 •

edited

khalidhuseynov commented Dec 27, 2016

cloverhearts Dec 20, 2016

cloverhearts Dec 20, 2016

khalidhuseynov commented Dec 30, 2016

AhyoungRyu commented Dec 30, 2016

khalidhuseynov commented Dec 30, 2016

AhyoungRyu commented Dec 30, 2016 •

edited

khalidhuseynov commented Dec 31, 2016

AhyoungRyu commented Dec 31, 2016

khalidhuseynov commented Jan 1, 2017

AhyoungRyu commented Jan 2, 2017

khalidhuseynov commented Jan 3, 2017

Leemoonsoo commented Jan 3, 2017

[ZEPPELIN-1745] View revisions in non-editable mode #1775

[ZEPPELIN-1745] View revisions in non-editable mode #1775

Conversation

khalidhuseynov commented Dec 16, 2016 • edited

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

bzz commented Dec 20, 2016 • edited

khalidhuseynov commented Dec 20, 2016

1ambda commented Dec 20, 2016

cloverhearts commented Dec 20, 2016

1ambda commented Dec 21, 2016

khalidhuseynov commented Dec 23, 2016

1ambda commented Dec 23, 2016

1ambda commented Dec 23, 2016 • edited

khalidhuseynov commented Dec 27, 2016

cloverhearts Dec 20, 2016

Choose a reason for hiding this comment

cloverhearts Dec 20, 2016

Choose a reason for hiding this comment

khalidhuseynov commented Dec 30, 2016

AhyoungRyu commented Dec 30, 2016

khalidhuseynov commented Dec 30, 2016

AhyoungRyu commented Dec 30, 2016 • edited

khalidhuseynov commented Dec 31, 2016

AhyoungRyu commented Dec 31, 2016

khalidhuseynov commented Jan 1, 2017

AhyoungRyu commented Jan 2, 2017

khalidhuseynov commented Jan 3, 2017

Leemoonsoo commented Jan 3, 2017

khalidhuseynov commented Dec 16, 2016 •

edited

bzz commented Dec 20, 2016 •

edited

1ambda commented Dec 23, 2016 •

edited

AhyoungRyu commented Dec 30, 2016 •

edited