[Zeppelin-945] Interpreter authorization by astroshim · Pull Request #1257 · apache/zeppelin

astroshim · 2016-08-02T06:56:54Z

What is this PR for?

This PR is derived from #1223

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-945

How should this be tested?

You can change user permission of interpreter at interpreter setting page.

Screenshots (if appropriate)

Questions:

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

AhyoungRyu · 2016-08-04T05:18:10Z

+ <div>
+ <p>
+ Enter comma separated interpreters in the fields. <br />
+ Empty field (*) implies anyone can run the interpreters.


It's a minor thing; how about this interpreter instead of the interpreters?

AhyoungRyu · 2016-08-04T05:32:00Z

@astroshim Awesome. I saw many people have talked about this feature :) I tested your patch locally, it works well as expected. But I found one issue regarding user suggestion,

The suggestion list doesn't disappear after pressing Enter key. Could you check this out?

astroshim · 2016-08-04T05:36:58Z

@AhyoungRyu Let me check. Thanks.

AhyoungRyu · 2016-08-04T05:37:15Z

@astroshim

Does this needs documentation? no

I think we need a documentation for "Interpreter Authorization" feature as well like we have below docs pages.

But separated PR would be okay i think :)

astroshim · 2016-08-04T05:52:20Z

@AhyoungRyu I fixed you pointed out and I'll create the document for this separated PR.
Thanks.

AhyoungRyu · 2016-08-04T05:56:12Z

+ if (event.keyCode === 13) {
+ closeAutoComplete();
+ }
+ else if (event.keyCode < 37 || event.keyCode > 40) { // arrow buttons.


Can we move this line to the above line? As google code style said https://google.github.io/styleguide/javaguide.html#s4.1.2-blocks-k-r-style. Maybe you can't build this with grunt because of disallowKeywordsOnNewLine error.

AhyoungRyu · 2016-08-04T06:04:13Z

@astroshim Tested again and checked the bug is gone. Thanks for the prompt fix 👍

bzz · 2016-08-04T08:37:17Z

+ var checkIfSelected = function() {
+ if (($scope.suggestions.length === 0) &&
+ ($scope.selectIndex < 0 || $scope.selectIndex >= $scope.suggestions.length) ||
+ ($scope.suggestions.length !== 0 && ($scope.selectIndex < 0 || $scope.selectIndex >= $scope.suggestions.length))


Long condition inside the if becomes quite hard to understand and follow here.
What do you think, could this be changed a bit to make it more readable i.e:
if(suggestionSelected($scope.suggestions.length,$scope.selectIndex)){//....}functionsuggestionSelected(len,i){return(len===0)&&(i<0||i>=len)||(len!==0&&(i<0||i>=len))}
And now it's easy to see how suggestionSelected can be further improved, keeping invariants.
Normalizing the parantheses according to logical operator precedence in javascript we get
return(len===0&&(i<0||i>=len))||(len!==0&&(i<0||i>=len))}
which, in turn, is the same as
return(i<0||i>=len)&&(len===0||len!==0)
and as the right side of the && is always true for an integer, we get
functionsuggestionSelected(len,i){returni<0||i>=len}
At the end, this might not worth extracting to a separate function, but this shows the benefits we can get from it.
The result is much simpler to understand and reason about.
@astroshim what do you think?

astroshim · 2016-08-04T10:30:31Z

Thank you for detail review @bzz and @felizbear
I'll fix the codes.

prabhjyotsingh · 2016-08-04T10:41:18Z

@astroshim A lot of these complications are already reduced with #1236 (which is already merged in master), take a look at it.

astroshim · 2016-08-04T10:59:13Z

@prabhjyotsingh Great. Thank you for giving me the guidance.

…ELIN-945_1

astroshim · 2016-08-06T13:08:00Z

\cc @AhyoungRyu @prabhjyotsingh @bzz @felizbear
I fixed UI like #1236. Please review.
Thank you!

prabhjyotsingh · 2016-08-06T14:25:03Z

I'll try it out, in the mean while, can you update screen-shot in description.

astroshim · 2016-08-06T16:32:21Z

@prabhjyotsingh I just updated screen-shot. Thank you for taking care of this.

prabhjyotsingh · 2016-08-08T05:20:12Z


-angular.module('zeppelinWebApp').controller('InterpreterCtrl',
+angular.module('zeppelinWebApp')
+.directive('interpreterDirective', function($timeout) {


can you move this directive to a different file zeppelin-web/src/components/*

I moved the directive. Thanks!

prabhjyotsingh · 2016-08-08T06:14:11Z

Except for minor fixes, looks good.

astroshim · 2016-08-09T04:36:00Z

@prabhjyotsingh Thank you for your detail review. I fixed all that you mentioned.

bzz · 2016-08-11T11:51:16Z

 this.host = host;
 }

+ public boolean isSetPermission() {


This is quite an ambiguous naming - usually in Java set\get used as a convention for getters and setters, so may be here we better call it like permissionIsSet() or permissionWasSet() - in this way we keep the almos-eglish readability in case of the code like
if (permissionIsSet()) { //.... }

bzz · 2016-08-11T13:31:55Z

@astroshim your changes look awesome to me!

There is one more improvement on Java side noted above, other than that - looks good to merge.

astroshim · 2016-08-15T15:55:03Z

@bzz please review and Thanks again.

bzz · 2016-08-16T05:06:40Z

@astroshim changes looks great to me.

There is one more comment above on isSetPermission naming, please let me know if you want to address this one as well, or want to keep it like that!

Will merge to master right after that, and if there is no further discussion

astroshim · 2016-08-16T05:40:07Z

@bzz I just fixed, I missed that.

bzz · 2016-08-16T05:58:21Z

Thank you @astroshim !

Merging to master if there is no further discussion.

bzz · 2016-08-16T07:01:24Z

CI fails on web-app \w networking issue :\ which is not relevant here

Could not transfer artifact org.codehaus.plexus:plexus-io:pom:2.1.3 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]

astroshim added 3 commits August 2, 2016 14:21

fit UX
407d260

fix js
29d8f43

fix for checking intpname
b34fdf7

AhyoungRyu reviewed Aug 4, 2016
View reviewed changes

fix for@AhyoungRyupointed.
1ae1c6a

AhyoungRyu reviewed Aug 4, 2016
View reviewed changes

bzz reviewed Aug 4, 2016
View reviewed changes

astroshim added 7 commits August 5, 2016 00:43

Merge branch 'master' ofhttps://github.com/apache/zeppelininto ZEPP…
635d523
…ELIN-945_1

wip
e56e5b1

wip
e1679b3

Merge branch 'master' into ZEPPELIN-945_1
e72c097

add interpreter create.
e1e7a35

fix initialize user value
e42cb9e

fix js
febce0c

prabhjyotsingh reviewed Aug 8, 2016
View reviewed changes

astroshim added 3 commits August 9, 2016 13:15

remove comment
f07542a

move directive to componets
3e25159

fix js code style
7ed8ad6

bzz reviewed Aug 11, 2016
View reviewed changes

code refactor.
2c48ded

fix naming.
83097ab

asfgit closed this in 43dc32bAug 16, 2016

Uh oh!

Conversation

astroshim commented Aug 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

AhyoungRyuAug 4, 2016

Choose a reason for hiding this comment

Uh oh!

AhyoungRyu commented Aug 4, 2016

Uh oh!

astroshim commented Aug 4, 2016

Uh oh!

AhyoungRyu commented Aug 4, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astroshim commented Aug 4, 2016

Uh oh!

AhyoungRyuAug 4, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AhyoungRyu commented Aug 4, 2016

Uh oh!

bzzAug 4, 2016

Choose a reason for hiding this comment

Uh oh!

astroshim commented Aug 4, 2016

Uh oh!

prabhjyotsingh commented Aug 4, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astroshim commented Aug 4, 2016

Uh oh!

astroshim commented Aug 6, 2016

Uh oh!

prabhjyotsingh commented Aug 6, 2016

Uh oh!

astroshim commented Aug 6, 2016

Uh oh!

prabhjyotsinghAug 8, 2016

Choose a reason for hiding this comment

Uh oh!

astroshimAug 9, 2016

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Aug 8, 2016

Uh oh!

astroshim commented Aug 9, 2016

Uh oh!

bzzAug 11, 2016

Choose a reason for hiding this comment

Uh oh!

bzz commented Aug 11, 2016

Uh oh!

astroshim commented Aug 15, 2016

Uh oh!

bzz commented Aug 16, 2016

Uh oh!

astroshim commented Aug 16, 2016

Uh oh!

bzz commented Aug 16, 2016

Uh oh!

bzz commented Aug 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

astroshim commented Aug 2, 2016•
edited
Loading

AhyoungRyu commented Aug 4, 2016•
edited
Loading

AhyoungRyuAug 4, 2016•
edited
Loading

prabhjyotsingh commented Aug 4, 2016•
edited
Loading