Warn if the title is a duplicate #10321

stacey-gammon · 2017-02-13T19:12:26Z

stacey-gammon · 2017-02-13T20:37:41Z

jenkins test this

LeeDr · 2017-02-13T22:39:47Z

Does this PR implement the same functionality for Saved Searches and Visualizations? I didn't do a thorough code review but noticed some dashboard files changed but not others that jumped out as Discover or Visualization changes.

I'm not sure the message is clear enough. It tells them that there already is a dashboard with this title. But it could be more clear about what will happen when I click Save dashboard. It's going to create another dashboard with the same name.

Maybe something like this?
A dashboard with the title 'New Dashboard' already exists. Would you like to make a duplicate with the same name?

But I need to think through the scenarios where I think this is going to appear.

one case, I'm editing an object that has been saved and I click Save, leave the existing name, don't check the Save as new ___ checkbox, and click Save.
This just updates the existing object with any changes. No need to show the dialog.
I'm editing an object that has been saved and I click Save, leave the existing name, and I check the Save as new ___ checkbox, and click Save. That should show your new dialog.
I create a new object and the first time I go to save it I enter the same name as an existing object (it could be the default name). That should show your new dialog. So I guess this is the same case as 2 above.

stacey-gammon · 2017-02-14T14:50:09Z

@LeeDr Updated the error message. I was already handling #1 and #3 but good catch on #2. Implemented that and added a test for it.

Yes the change is implemented across the board for all objects, as the important changes are in saved_object.js. I only added tests for the dashboard.

stacey-gammon · 2017-02-14T16:30:48Z

Hmmm, my query is not correct as it returns matches on substrings. Trying to figure out how to create an exact match query...

stacey-gammon · 2017-02-15T13:55:45Z

jenkins test this

(looks like the build was aborted)

stacey-gammon · 2017-02-16T15:23:04Z

Hmm, latest build failure is related to saving, but not sure it's due to this PR:

15:12:57.770: --- tryForTime failure: [POST http://localhost:4444/wd/hub/session/251abda956b6c9bad5a51ef85b848bc7/element / {"using":"link text","value":"Visualization DataTable"}] no such element: Unable to locate element: {"method":"link text","selector":"Visualization DataTable"}
  (Session info: chrome=55.0.2883.87)
  (Driver info: chromedriver=2.24.417424 (c5c5ea873213ee72e3d0929b47482681555340c3),platform=Linux 3.13.0-74-generic x86_64)
15:13:08.328: --- tryForTime failed again with the same message  ...
15:13:18.888: --- tryForTime failed again with the same message  ...
15:13:29.451: --- tryForTime failed again with the same message  ...
15:13:30.017: Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/screenshots/failure/failure_1487258010016_data table_should be able to save and load.png"
>> FAIL: chrome on any platform - kibana - visualize app - visualize app - data table - should be able to save and load (49214ms)
Error: tryForTime timeout: [POST http://localhost:4444/wd/hub/session/251abda956b6c9bad5a51ef85b848bc7/element / {"using":"link text","value":"Visualization DataTable"}] no such element: Unable to locate element: {"method":"link text","selector":"Visualization DataTable"}
  (Session info: chrome=55.0.2883.87)
  (Driver info: chromedriver=2.24.417424 (c5c5ea873213ee72e3d0929b47482681555340c3),platform=Linux 3.13.0-74-generic x86_64)
    at runRequest (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/leadfoot/Session.js:88:40)
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/node_modules/leadfoot/Session.js:109:39

I'll rebase

cjcenizal

Functionality looks great! Just had some minor code suggestions.

cjcenizal · 2017-02-16T20:22:27Z

src/ui/public/courier/saved_object/saved_object.js

+            err.message === OVERWRITE_REJECTED ||
+            err.message === SAVE_DUPLICATE_REJECTED)) {
+            return;
+          }


Can we break this up to make it a little more readable?

if (err) { if (err.message === OVERWRITE_REJECTED) { return; } if (err.message === SAVE_DUPLICATE_REJECTED) { return; } }

ehhh, I don't love suggestion, I'd look at that and be like, why not just combine them into an ||?

how about this new variation as a compromise?

cjcenizal · 2017-02-16T20:23:59Z

src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html

@@ -3,7 +3,7 @@
    In previous versions of Kibana, changing the name of a {{savedObject.getDisplayName()}} would make a copy with the new name. Use the 'Save as a new {{savedObject.getDisplayName()}}' checkbox to do this now.
  </div>
  <label>
-    <input type="checkbox" ng-model="savedObject.copyOnSave" ng-checked="savedObject.copyOnSave">
+    <input type="checkbox" data-test-subj="saveAsNewCheckbox" ng-model="savedObject.copyOnSave" ng-checked="savedObject.copyOnSave">


Can we put these attributes on multiple lines?

<input type="checkbox" data-test-subj="saveAsNewCheckbox" ng-model="savedObject.copyOnSave" ng-checked="savedObject.copyOnSave" >

cjcenizal · 2017-02-16T20:29:46Z

test/functional/apps/dashboard/_dashboard_save.js

+    expect(countOfDashboards).to.equal(2);
+  });
+
+  bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() {


I had a hard time parsing this description... how does this sound to you: "Does not warn when you save an existing dashboard with the title it already has, and that title is a duplicate"? Does that describe this test accurately?

cjcenizal · 2017-02-16T20:31:18Z

test/functional/apps/dashboard/_dashboard_save.js

+    expect(isConfirmOpen).to.equal(false);
+  });
+
+  bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() {


And here, how about: "Warns you when you Save as New Dashboard, and the title is a duplicate"?

cjcenizal · 2017-02-16T20:32:35Z

test/functional/index.js

      'intern/dojo/node!./apps/dashboard',
-      'intern/dojo/node!./status_page'
+    //  'intern/dojo/node!./status_page'


Haha I do this all the time! 😄

This time it was actually intentional :) Trying to reduce the time for a build because I was having a very tough time figuring out what the issue was (turns out it was a delay between saving a new dashboard and reloading the url to point to the newly saved dashboard).

cjcenizal · 2017-02-16T20:34:32Z

test/support/page_objects/dashboard_page.js

    PageObjects.common.debug('Storing time with dashboard: ' + on);
+    const saveAsNewCheckbox = await PageObjects.common.findTestSubject('saveAsNewCheckbox');
+    const checked = await saveAsNewCheckbox.getProperty('checked');
+    if (checked === true && on === false ||


Can this be made terser?

if (checked !== isAlreadyChecked) { PageObjects.common.debug('Flipping save as new checkbox'); await saveAsNewCheckbox.click(); }

Doh! Good catch, much clearer. :)

cjcenizal · 2017-02-16T20:36:16Z

test/support/page_objects/dashboard_page.js

@@ -276,8 +319,19 @@ export default class DashboardPage {
    await PageObjects.header.setAbsoluteRange(fromTime, toTime);
  }

-  async storeTimeWithDashboard(on) {
+  async saveAsNewCheckbox(on) {


Can we rename this to setSaveAsNewCheckBox(checked) to make it clear what the action is that we're taking?

cjcenizal · 2017-02-16T20:36:48Z

test/support/page_objects/dashboard_page.js

    PageObjects.common.debug('Storing time with dashboard: ' + on);
+    const saveAsNewCheckbox = await PageObjects.common.findTestSubject('saveAsNewCheckbox');
+    const checked = await saveAsNewCheckbox.getProperty('checked');


Can we rename this to isAlreadyChecked?

cjcenizal · 2017-02-17T18:14:52Z

src/ui/public/courier/saved_object/saved_object.js

+ * @param error {Error} the error
+ * @return {boolean}
+ */
+function getShouldSuppressExpectedError(error) {


How about renaming this to isErrorNonFatal? This way the name of function helps us assign meaning to the error without specifying what we should do about it. If for some reason we decide to return the error instead of suppressing it, this function will still be useful and won't need to be renamed.

I think that's a little confusing too because it's not an error at all, it's a user response. The whole flow is a bit awkward, and it'd be better if we just stopped using the confirmModalPromise to begin with, but I don't want to do that refactor now.

I have no better suggestion, so sgtm for now.

stacey-gammon · 2017-02-23T17:04:49Z

@cjcenizal or @LeeDr, any more comments on this PR? I think I addressed everything.

LeeDr · 2017-02-23T18:37:10Z

I just tried a case with saving Visualizations where it didn't work, probably because of the characters I used?

I saved a search as ----- #####
I saved it again and checked the box Save as a new visualization
I should have gotten the message that it already exists, but I didn't.

Obviously this is an odd case, and I only tried this one visualization name so far. So I don't know exactly what cases work and which don't. Are we comparing analyzed names which would remove those chars?

I'll keep testing a bit more.

cjcenizal

LGTM!

LeeDr · 2017-02-23T18:45:03Z

Also if I name an object ///// I can create more of them with no warnings.

But combinations of special characters and letter always seems to work correctly.
- test - name gives a warning if I try to make a copy.
and test - name doesn't incorrectly give a warning matching - test - name.

And I also tested that the duplication check is only within that object type. So I can create a saved search, visualization, and dashboard all with the name test1 with no warnings (as I would expect).

If the special character cases aren't easy to solve, I'd still give a LGTM.

add more tests.

Add a comment and rename one of the variables to make the distinction clearer.

…r the page has navigated away

stacey-gammon · 2017-02-23T19:56:16Z

Unfortunately it's not easy to fix @LeeDr. If I have a visualization name /// and I run this query in devtools:

GET _search
{
  "query": {
    "bool": {
     "must": { "match_phrase": {  "title": "///" } }
    }
  }
}

Nothing is returned, presumably because it strips special characters.

If this issue were resolved: #10348 then we could get around this shortcoming.

For the short term, I think we should acknowledge it as a limitation, but move forward.

LeeDr

LGTM Thanks / for - checking # on the special ! chars :-)

Backports PR #10321 **Commit 1:** Warn if the title is a duplicate * Original sha: b0aa0b8 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-13T18:07:23Z **Commit 2:** warn when copyOnSave is checked. add a test * Original sha: ee42002 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-14T14:20:37Z **Commit 3:** update message * Original sha: c90c8ae * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-14T14:24:50Z **Commit 4:** fix tests * Original sha: 5ec1a92 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-14T15:04:14Z **Commit 5:** Fix issues with substring matching add more tests. * Original sha: 4335e9e * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-14T18:54:58Z **Commit 6:** make case insensitive and return matching title * Original sha: 8881739 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-14T19:34:04Z **Commit 7:** Fix bug caused by subtle difference between this.type and type Add a comment and rename one of the variables to make the distinction clearer. * Original sha: f4011ce * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-16T16:31:13Z **Commit 8:** add debug messages. * Original sha: 2d44889 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-16T18:05:03Z **Commit 9:** fix tests again * Original sha: 3212c91 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-16T22:10:25Z **Commit 10:** uncomment out tests * Original sha: 1f2cb28 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-16T22:12:06Z **Commit 11:** more debug messages * Original sha: 807b190 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-17T02:09:37Z **Commit 12:** save more screenshots * Original sha: 3d025e9 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-17T02:34:29Z **Commit 13:** Need to wait till save finishes or the page may redirect the url after the page has navigated away * Original sha: de00e24 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-17T14:01:22Z **Commit 14:** address code review comments * Original sha: 7bc0268 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-02-17T17:28:34Z

stacey-gammon requested review from cjcenizal and LeeDr February 13, 2017 19:13

stacey-gammon force-pushed the warn-duplicate-title branch from 2454c0b to b5c3fbf Compare February 13, 2017 21:18

stacey-gammon added the v5.4.0 label Feb 14, 2017

stacey-gammon force-pushed the warn-duplicate-title branch from 7cd051a to e3aedf3 Compare February 14, 2017 18:59

stacey-gammon mentioned this pull request Feb 14, 2017

Add Tests for "Add new Visualization" link in dashboard #10349

Closed

tbragin added the :Sharing label Feb 14, 2017

stacey-gammon added the Feature:Dashboard Dashboard related features label Feb 14, 2017

stacey-gammon force-pushed the warn-duplicate-title branch 6 times, most recently from c7540dd to 76b9bdb Compare February 16, 2017 14:48

stacey-gammon force-pushed the warn-duplicate-title branch 3 times, most recently from 973b6c5 to f661370 Compare February 16, 2017 20:16

cjcenizal suggested changes Feb 16, 2017

View reviewed changes

stacey-gammon force-pushed the warn-duplicate-title branch 2 times, most recently from 0eae3c7 to c1e2f59 Compare February 17, 2017 17:31

cjcenizal reviewed Feb 17, 2017

View reviewed changes

stacey-gammon force-pushed the warn-duplicate-title branch 3 times, most recently from a0c8896 to a0c5daf Compare February 22, 2017 17:02

stacey-gammon force-pushed the warn-duplicate-title branch from 2d3b1b6 to 75d9e46 Compare February 23, 2017 16:25

cjcenizal approved these changes Feb 23, 2017

View reviewed changes

stacey-gammon added 14 commits February 23, 2017 14:43

Warn if the title is a duplicate

b0aa0b8

warn when copyOnSave is checked. add a test

ee42002

update message

c90c8ae

fix tests

5ec1a92

Fix issues with substring matching

4335e9e

add more tests.

make case insensitive and return matching title

8881739

Fix bug caused by subtle difference between this.type and type

f4011ce

Add a comment and rename one of the variables to make the distinction clearer.

add debug messages.

2d44889

fix tests again

3212c91

uncomment out tests

1f2cb28

more debug messages

807b190

save more screenshots

3d025e9

Need to wait till save finishes or the page may redirect the url afte…

de00e24

…r the page has navigated away

address code review comments

7bc0268

stacey-gammon force-pushed the warn-duplicate-title branch from 75d9e46 to 7bc0268 Compare February 23, 2017 19:43

LeeDr approved these changes Feb 23, 2017

View reviewed changes

stacey-gammon merged commit c253583 into elastic:master Feb 24, 2017

elastic-jasper mentioned this pull request Feb 24, 2017

[5.x] Warn if the title is a duplicate #10566

Closed

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Feb 24, 2017

Backport elastic#10321

7f147f1

stacey-gammon mentioned this pull request Feb 24, 2017

[5.x] Backport warn on duplicate #10568

Merged

stacey-gammon added a commit that referenced this pull request Feb 25, 2017

Backport #10321 (#10568)

1c414d0

stacey-gammon deleted the warn-duplicate-title branch April 6, 2017 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if the title is a duplicate #10321

Warn if the title is a duplicate #10321

stacey-gammon commented Feb 13, 2017 •

edited

Loading

stacey-gammon commented Feb 13, 2017

LeeDr commented Feb 13, 2017

stacey-gammon commented Feb 14, 2017

stacey-gammon commented Feb 14, 2017

stacey-gammon commented Feb 15, 2017

stacey-gammon commented Feb 16, 2017 •

edited

Loading

cjcenizal left a comment

cjcenizal Feb 16, 2017

stacey-gammon Feb 17, 2017

cjcenizal Feb 16, 2017

cjcenizal Feb 16, 2017

cjcenizal Feb 16, 2017

cjcenizal Feb 16, 2017

stacey-gammon Feb 17, 2017

cjcenizal Feb 16, 2017

stacey-gammon Feb 17, 2017

cjcenizal Feb 16, 2017

cjcenizal Feb 16, 2017

cjcenizal Feb 17, 2017

stacey-gammon Feb 17, 2017

stacey-gammon commented Feb 23, 2017

LeeDr commented Feb 23, 2017

cjcenizal left a comment

LeeDr commented Feb 23, 2017

stacey-gammon commented Feb 23, 2017

LeeDr left a comment

Warn if the title is a duplicate #10321

Warn if the title is a duplicate #10321

Conversation

stacey-gammon commented Feb 13, 2017 • edited Loading

stacey-gammon commented Feb 13, 2017

LeeDr commented Feb 13, 2017

stacey-gammon commented Feb 14, 2017

stacey-gammon commented Feb 14, 2017

stacey-gammon commented Feb 15, 2017

stacey-gammon commented Feb 16, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Feb 23, 2017

LeeDr commented Feb 23, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

LeeDr commented Feb 23, 2017

stacey-gammon commented Feb 23, 2017

LeeDr left a comment

Choose a reason for hiding this comment

stacey-gammon commented Feb 13, 2017 •

edited

Loading

stacey-gammon commented Feb 16, 2017 •

edited

Loading