Skip to content

Conversation

@astroshim
Copy link
Contributor

What is this PR for?

This PR is for giving proper information to users and blocking unnecessary communication with zeppelin server when trying to delete last paragraph because the last paragraph can't be deleted.

What type of PR is it?

Improvement

What is the Jira issue?

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

How should this be tested?

You can try to remove last paragraph of a notebook.

Screenshots (if appropriate)

image

Questions:

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

@cloverhearts
Copy link
Member

tested.
LGTM +1

@corneadoug
Copy link
Contributor

I would like to suggest a different approach:
Not showing the delete button if the paragraph is the last one.

It is also quite easy to do:
Just need to add ng-hide="$last" to the remove link in paragraph-control.html

screen shot 2016-06-10 at 3 43 12 pm

Since angular has both $first and $last, we could even remove 'Move Up' or 'Move Down' option depending on whether the paragraph is the first or the last.

@astroshim
Copy link
Contributor Author

@corneadoug Thank you very much for your review and opinion. I'll follow your advice.

@minahlee
Copy link
Member

@corneadoug I like the idea, but how do we want to handle paragraph deletion by keyboard shortcut in this case?

@corneadoug
Copy link
Contributor

@minahlee Maybe then in that case it would be needed

@astroshim
Copy link
Contributor Author

@minahlee @corneadoug Then we need two ways to solve this issue?

@corneadoug
Copy link
Contributor

I guess both methods can cohabitate then

@astroshim
Copy link
Contributor Author

Thank you for clarification.

@astroshim
Copy link
Contributor Author

@corneadoug @minahlee I fixed the code to hide "Remove" menu on the last paragraph menu. please review.

@minahlee
Copy link
Member

minahlee commented Jun 10, 2016

Tested, looks good. How about applying @corneadoug's opinion making the first paragraph not to show Move Up, last paragraph not to show Move Down?
Plus, can you take care of test failure?

Tests run: 8, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 162.898 sec <<< FAILURE! - in org.apache.zeppelin.integration.ParagraphActionsIT
testRemoveButton(org.apache.zeppelin.integration.ParagraphActionsIT)  Time elapsed: 36.193 sec  <<< ERROR!
org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@class='modal-dialog'][contains(.,'delete this paragraph')]//div[@class='modal-footer']//button[contains(.,'OK')]"}

@astroshim
Copy link
Contributor Author

@minahlee I'll take care about that you mentioned. thanks.

@astroshim
Copy link
Contributor Author

@minahlee Could you tell me how can i get your test failure?

@corneadoug
Copy link
Contributor

@astroshim She posted the test failure.
My guess is that the Notebook has only one paragraph, therefore the xpath is on the last paragraph.
Since we removed the delete element from the last paragraph, the test fail.

The exact failing tests and the error lines:

ParagraphActionsIT.testRemoveButton:153
ParagraphActionsIT.testCreateNewButton:86

@astroshim
Copy link
Contributor Author

@corneadoug Thank you for your kind explanation.
I know the reason about the error but I couldn't reproduce the error even if i build like mvn clean package.
Did you build also like i did?

@corneadoug
Copy link
Contributor

You need to trigger the selenium test locally, it doesn't by default without some configuration.
(TEST_SELENIUM="true" as env)
Otherwise you can just push your changes and let the CI do it

@astroshim
Copy link
Contributor Author

@corneadoug Okay. Thank you!!

@corneadoug
Copy link
Contributor

Tested Selenium Tests locally from original commit:
6f181ef

And it's working well.

I will create a flacky test issue on JIRA,
@astroshim Could you rewrite history back to the commit I just Mentioned? After that LGTM

@astroshim
Copy link
Contributor Author

@corneadoug I really appreciate for your effort. Thank you. I reset my commits.

@corneadoug
Copy link
Contributor

Merging if there's no more discussions

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants