add option to confirm overwrite on save #9659

stacey-gammon · 2016-12-27T19:44:49Z

fixes #9589 and adds tests

tsullivan · 2016-12-27T20:59:24Z

src/core_plugins/kibana/public/management/sections/objects/_objects.js

@@ -158,7 +158,7 @@ uiModules.get('apps/management')
          return service.get().then(function (obj) {
            obj.id = doc._id;
            return obj.applyESResp(doc).then(function () {
-              return obj.save();
+              return obj.save(true);


I usually try to stick to having an options object parameter in a function definition, and have the calling code use something like:

obj.save({ confirmOverwrite: true });

It helps readability a ton, because a simple boolean parameter is always ambiguous. There are a lot of things the reader might think it means.

makes a lot of sense, I like it. Updated!

tsullivan · 2016-12-27T21:11:25Z

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

+
+    /**
+     * @typedef {Object} SaveOptions
+     * @property {boolean} confirmOverwrite=false - If true, attempts to create the source so it


minor, but the comment isn't accurate any more

stacey-gammon · 2016-12-27T21:45:15Z

jenkins test this

Test failure looks irrelevant to this PR and probably a transitory failure:

ERROR: Build step failed with exception
com.amazonaws.SdkClientException: Unable to execute HTTP request: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:972)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:676)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:650)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.execute(AmazonHttpClient.java:633)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.access$300(AmazonHttpClient.java:601)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutionBuilderImpl.execute(AmazonHttpClient.java:583)
	at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:447)
	at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:4137)
	at com.amazonaws.services.s3.AmazonS3Client.putObject(AmazonS3Client.java:1685)
	at com.amazonaws.services.s3.transfer.internal.UploadCallable.uploadInOneChunk(UploadCallable.java:133)
	at com.amazonaws.services.s3.transfer.internal.UploadCallable.call(UploadCallable.java:125)
	at com.amazonaws.services.s3.transfer.internal.UploadMonitor.call(UploadMonitor.java:139)
	at com.amazonaws.services.s3.transfer.internal.UploadMonitor.call(UploadMonitor.java:47)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
	at ......remote call to Channel to /10.34.148.136(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1537)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)
	at hudson.remoting.Channel.call(Channel.java:822)
	at hudson.FilePath.act(FilePath.java:985)
	at hudson.FilePath.act(FilePath.java:974)
	at hudson.plugins.s3.S3Profile.invoke(S3Profile.java:201)
	at hudson.plugins.s3.S3Profile.waitUploads(S3Profile.java:195)
	at hudson.plugins.s3.S3Profile.upload(S3Profile.java:174)
	at hudson.plugins.s3.S3BucketPublisher.perform(S3BucketPublisher.java:198)
	at hudson.tasks.BuildStepCompatibilityLayer.perform(BuildStepCompatibilityLayer.java:78)
	at hudson.tasks.BuildStepMonitor$2.perform(BuildStepMonitor.java:32)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:720)
	at hudson.model.Build$BuildExecution.post2(Build.java:185)
	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:665)
	at hudson.model.Run.execute(Run.java:1754)
	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)

tsullivan · 2016-12-29T16:59:38Z

I haven't pulled this in to test, but it looks good to me code-wise

w33ble · 2016-12-29T17:09:37Z

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

+        .catch((err) => {
+          // record exists, confirm overwriting
+          if (_.get(err, 'origError.status') === 409) {
+            const confirmMessage = 'Are you sure you want to overwrite ' + this.title + '?';


this.title here is actually the title of the imported visualization, not the original one, so if the title in the imported JSON is different, the message is wrong. I poked at this quickly, but I'm not sure how to get the title of the record being replaced...

Huh, worse than the message being wrong, it will actually warn me it's going to override a document, and then it doesn't. Is the import still matching on the title instead of the id?

The import is matching on the id, not the title.

Good catch on the error message. What if it said Are you sure you want to overwrite the object with id 50416250-cdf6-11e6-8394-6511ff44915a and replace it with Newest Dashboard??

Pretty ugly and I hate exposing the id. :( I suppose I could change the logic and make a get query to es before the doCreate and check for document existence that way, but that might be overkill.

For what it's worth, this preserves the old logic which did the same exact thing, so at least we are no worse off than we were before. I'm leaning towards leaving it as is and filing an issue to improve the error message as a separate issue. This is at least better than no overwrite warning.

w33ble · 2016-12-29T18:21:11Z

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

+    // the test environment to hang.
+    function ($provide) {
+      $provide.decorator('safeConfirm', () => {
+        return (message) => {


"If your arrow function is only returning an object literal, then wrap the object in paranthesis rather than using an explicit return"

https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#arrow-functions

Interesting, I didn't know you could do that. But does that fit here? I'm returning a function, not an object literal.

nevermind, looks like it works on any single return, not just object literal. Though I wonder if the style guide applies to all or just object literals. Anyway, updated.

w33ble · 2016-12-29T18:23:24Z

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

+            const confirmMessage = 'Are you sure you want to overwrite ' + this.title + '?';
+
+            return safeConfirm(confirmMessage).then(
+              () => { return docSource.doIndex(source); },


https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#arrow-functions

The styleguide seems to prefer implicit returns. I'd prefer to see this as a .then/.catch, but it's your call.

done! Didn't know about that.

w33ble · 2016-12-29T18:23:43Z

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

+
+            return safeConfirm(confirmMessage).then(
+              () => { return docSource.doIndex(source); },
+              () => { throw { confirmRejected : true }; }


Throwing an object? Why not an error?

guess I didn't want to do a string comparison, but it does look a bit weird. Done.

w33ble · 2016-12-29T20:44:34Z

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

@@ -88,11 +88,11 @@ describe('Saved Object', function () {
    // The default implementation of safeConfirm uses $timeout which will cause
    // the test environment to hang.
    function ($provide) {
-      $provide.decorator('safeConfirm', () => {
-        return (message) => {
+      $provide.decorator('safeConfirm', () =>


I think this got really confusing now. The shortest form of what you have here would be:

$provide.decorator('safeConfirm', () => message => window.confirm(message) ? Promise.resolve() : Promise.reject());

If you're against the implicit return of a function (and I can see why you might be), something like this would also work:

const resolveConfirm = message => window.confirm(message) ? Promise.resolve() : Promise.reject(); $provide.decorator('safeConfirm', () => resolveConfirm);

ah right, two implicit returns. :) yea, this is getting confusing, opted for the second version.

w33ble · 2016-12-29T20:52:11Z

Our conversation on the importing got collapsed :(

Good catch on the error message. What if it said Are you sure you want to overwrite the object with id 50416250-cdf6-11e6-8394-6511ff44915a and replace it with Newest Dashboard?? Pretty ugly and I hate exposing the id. :(

I agree, exposing the ID isn't right either, we don't show that to the user, and it means nothing, so it's not helpful. As you point out, this isn't a new problem, I'm fine punting on it. I'll make a new issue.

The import is matching on the id, not the title.

Strange. I don't know what I did differently, the copying of saved objects isn't happening for me anymore now. Previously I did this:

Export a saved vis
Edit the exported JSON and change the title
Import the JSON file

I was prompted about overwriting the saved vis, which is expected, but when it got imported, I now had 2 saved visualizations, the old one and the renamed one I just imported.

stacey-gammon · 2016-12-29T20:57:22Z

@w33ble That is really strange. So you are saying it wasn't working at first, but now you can't repro and it is working as expected? Is it possible you accidentally altered the id when you were changing the title? I know it sounds unlikely but those steps work as expected for me. If you can still repro the issue, let me know.

w33ble · 2016-12-29T21:04:16Z

Correct, it was wonky before, but now I can't get it to happen again. I only had 1 saved thing, and only changed the title. I have no idea why the ID would have changed... maybe I updated the saved ID some other way or something? But that wouldn't really explain why I got an overwrite notification.

So strange, but since it's not happening anymore, 🤷‍♂️

w33ble · 2016-12-29T21:15:48Z

LGTM!

stacey-gammon · 2016-12-30T14:51:35Z

@ycombinator or @tsullivan, one of you able to give this a final look today? 🙏

tsullivan · 2016-12-30T15:59:45Z

conflicts in the branch

- implicit returns from arrow function - throw an error not an object clean up implicit return confusion use promise.reject instead of throw.

tsullivan · 2016-12-30T16:28:17Z

Export a saved vis

Edit the exported JSON and change the title

Import the JSON file

FWIW I tried this (using a saved search instead of a vis), and the saved search I had in my system was overwritten with a saved search having the new title

tsullivan · 2016-12-30T16:36:00Z

This is probably minor, but when I tried the steps above, the confirmation asks me if I want to overwrite the saved search, but it uses the changed name of the search in the confirm dialog.

The saved search in my system is titled Avocado Tweets, and the saved search I'm importing with the same ID is titled Avocado Tweets - RENAMED.

Could the message instead use the title of my existing search?

stacey-gammon · 2016-12-30T17:06:48Z

@tsullivan Unfortunately it's not very simple to use the original name - I'd have to modify the es query to retrieve it. @w33ble mentioned the same issue above and created another ticket for it (#9680).
This is how Kibana always functioned, it's not a regression bug, so I decided to leave as is for this PR and address that issue separately.

tsullivan · 2016-12-30T17:07:56Z

LGTM!

stacey-gammon · 2016-12-30T17:15:43Z

aborted build

Aborted by unknown
Publish artifacts to S3 Bucket Skipping publishing on S3 because build aborted
Finished: ABORTED

jenkins test this

Backports PR #9659 **Commit 1:** add option to confirm overwrite on save * Original sha: 5875c15 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-27T14:29:48Z **Commit 2:** Make save options instead of using a boolean to increase readability. * Original sha: 1701b9c * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-27T21:07:56Z **Commit 3:** clean up comment * Original sha: bc2b9b1 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-27T21:14:43Z **Commit 4:** Address code comments - implicit returns from arrow function - throw an error not an object clean up implicit return confusion use promise.reject instead of throw. * Original sha: 8e43346 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-29T19:11:50Z

* add option to confirm overwrite on save * Make save options instead of using a boolean to increase readability. * clean up comment * Address code comments - implicit returns from arrow function - throw an error not an object clean up implicit return confusion use promise.reject instead of throw.

stacey-gammon requested a review from kobelb December 27, 2016 20:24

tsullivan reviewed Dec 27, 2016

View reviewed changes

stacey-gammon force-pushed the confirm-overwrite-on-save branch from defa527 to 8c31c69 Compare December 28, 2016 15:30

stacey-gammon removed the request for review from kobelb December 28, 2016 17:51

stacey-gammon added review v5.2.0 labels Dec 29, 2016

w33ble assigned ycombinator and w33ble Dec 29, 2016

w33ble added the v6.0.0 label Dec 29, 2016

w33ble assigned tsullivan Dec 29, 2016

tsullivan removed their assignment Dec 29, 2016

w33ble suggested changes Dec 29, 2016

View reviewed changes

w33ble mentioned this pull request Dec 29, 2016

Overwriting imported saved objects shows the wrong title #9680

Closed

stacey-gammon force-pushed the confirm-overwrite-on-save branch from 159052f to e54a246 Compare December 29, 2016 21:01

stacey-gammon force-pushed the confirm-overwrite-on-save branch from e54a246 to 5e7240e Compare December 29, 2016 21:13

w33ble approved these changes Dec 29, 2016

View reviewed changes

stacey-gammon added 4 commits December 30, 2016 11:12

add option to confirm overwrite on save

5875c15

Make save options instead of using a boolean to increase readability.

1701b9c

clean up comment

bc2b9b1

Address code comments

8e43346

- implicit returns from arrow function - throw an error not an object clean up implicit return confusion use promise.reject instead of throw.

stacey-gammon force-pushed the confirm-overwrite-on-save branch from 5e7240e to 8e43346 Compare December 30, 2016 16:14

tsullivan approved these changes Dec 30, 2016

View reviewed changes

stacey-gammon merged commit e75e2a2 into elastic:master Dec 30, 2016

elastic-jasper mentioned this pull request Jan 2, 2017

[5.x] add option to confirm overwrite on save #9695

Closed

elastic-jasper mentioned this pull request Jan 3, 2017

[5.x] add option to confirm overwrite on save #9709

Closed

tylersmalley mentioned this pull request Jan 3, 2017

[5.x] add option to confirm overwrite on save #9711

Merged

tbragin added the :Sharing label Jan 14, 2017

stacey-gammon deleted the confirm-overwrite-on-save branch January 19, 2017 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to confirm overwrite on save #9659

add option to confirm overwrite on save #9659

stacey-gammon commented Dec 27, 2016

tsullivan Dec 27, 2016

stacey-gammon Dec 27, 2016

tsullivan Dec 27, 2016

stacey-gammon Dec 27, 2016

stacey-gammon commented Dec 27, 2016 •

edited

tsullivan commented Dec 29, 2016

w33ble Dec 29, 2016

w33ble Dec 29, 2016

stacey-gammon Dec 29, 2016

w33ble Dec 29, 2016

stacey-gammon Dec 29, 2016

stacey-gammon Dec 29, 2016

w33ble Dec 29, 2016

stacey-gammon Dec 29, 2016

w33ble Dec 29, 2016

stacey-gammon Dec 29, 2016

w33ble Dec 29, 2016

stacey-gammon Dec 29, 2016

w33ble commented Dec 29, 2016

stacey-gammon commented Dec 29, 2016

w33ble commented Dec 29, 2016

w33ble commented Dec 29, 2016

stacey-gammon commented Dec 30, 2016

tsullivan commented Dec 30, 2016

tsullivan commented Dec 30, 2016

tsullivan commented Dec 30, 2016

stacey-gammon commented Dec 30, 2016

tsullivan commented Dec 30, 2016

stacey-gammon commented Dec 30, 2016

add option to confirm overwrite on save #9659

add option to confirm overwrite on save #9659

Conversation

stacey-gammon commented Dec 27, 2016

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 Dec 27, 2016 • edited

tsullivan commented Dec 29, 2016

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

w33ble commented Dec 29, 2016

stacey-gammon commented Dec 29, 2016

w33ble commented Dec 29, 2016

w33ble commented Dec 29, 2016

stacey-gammon commented Dec 30, 2016

tsullivan commented Dec 30, 2016

tsullivan commented Dec 30, 2016

tsullivan commented Dec 30, 2016

stacey-gammon commented Dec 30, 2016

tsullivan commented Dec 30, 2016

stacey-gammon commented Dec 30, 2016

stacey-gammon commented Dec 27, 2016 •

edited