Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save/rename flow fix #9087

Merged
merged 9 commits into from
Dec 2, 2016
2 changes: 1 addition & 1 deletion src/core_plugins/kibana/public/dashboard/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
};

$scope.save = function () {
$state.title = dash.id = dash.title;
$state.title = dash.title;
$state.save();

const timeRestoreObj = _.pick(timefilter.refreshInterval, ['display', 'pause', 'section', 'value']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
role="form"
ng-submit="opts.save()"
>
<div class="localDropdownTitle">Save Dashboard</div>
<div class="localDropdownTitle">Save {{opts.dashboard.getDisplayName()}}</div>
<input
class="localDropdownInput"
id="dashboardTitle"
Expand All @@ -11,12 +11,18 @@
placeholder="Dashboard title"
input-focus="select"
>

<saved-object-save-as-check-box saved-object="opts.dashboard"></saved-object-save-as-check-box>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing a slight flicker when saving a Dashboard (or other object) for the first time. You can see the "Save as a new dashboard" checkbox flicker for a moment before the dropdown is closed:

save_as_flicker

I think we just need to add a "isSaving" flag to the inherited scope. It will default to false, we can set it to true while the save request is being sent, and then back to false once the response is received.

Then we can pass it to the savedObjectSaveAsCheckBox directive:

<saved-object-save-as-check-box
  saved-object="opts.dashboard"
  is-saving="opts.isSaving"
></saved-object-save-as-check-box>

The directive can then hide itself when isSave is true:

<div ng-hide="!savedObject.id || isSaving">
  <!-- ... -->
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice catch! I opted to throw the variable into savedObject so I wouldn't have to add that input parameter to every user of the check box.


<div class="form-group">
<label>
<input type="checkbox" ng-model="opts.dashboard.timeRestore" ng-checked="opts.dashboard.timeRestore">
Store time with dashboard
<kbn-info info="Change the time filter to the currently selected time each time this dashboard is loaded"></kbn-info>
Store time with {{opts.dashboard.getDisplayName()}}
Copy link
Contributor

@cjcenizal cjcenizal Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the next line to:

<kbn-info placement="bottom" info="Change the time filter to the currently selected time each time this dashboard is loaded"></kbn-info>

Currently, this tooltip gets cropped:

image

<kbn-info placement="bottom" info="Change the time filter to the currently selected time each time this dashboard is loaded"></kbn-info>
</label>
</div>
<button type="submit" ng-disabled="!opts.dashboard.title" class="btn btn-primary" aria-label="Save dashboard">Save</button>

<button type="submit" ng-disabled="!opts.dashboard.title" class="btn btn-primary" aria-label="Save dashboard">
Save
</button>
</form>
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ function discoverController($scope, config, courier, $route, $window, Notifier,
$scope.opts.saveDataSource = function () {
return $scope.updateDataSource()
.then(function () {
savedSearch.id = savedSearch.title;
savedSearch.columns = $scope.state.columns;
savedSearch.sort = $scope.state.sort;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
input-focus="select"
placeholder="Name this search..."
>

<saved-object-save-as-check-box saved-object="opts.savedSearch"></saved-object-save-as-check-box>
<button ng-disabled="!opts.savedSearch.title" data-test-subj="discover-save-search-btn" type="submit" class="btn btn-primary">
Save
</button>
Expand Down
1 change: 0 additions & 1 deletion src/core_plugins/kibana/public/visualize/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,6 @@ function VisEditor($scope, $route, timefilter, AppState, $location, kbnUrl, $tim
* Called when the user clicks "Save" button.
*/
$scope.doSave = function () {
savedVis.id = savedVis.title;
// vis.title was not bound and it's needed to reflect title into visState
$state.vis.title = savedVis.title;
savedVis.visState = $state.vis;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
ng-model="opts.savedVis.title"
required
>

<saved-object-save-as-check-box saved-object="opts.savedVis"></saved-object-save-as-check-box>

<button
data-test-subj="saveVisualizationButton"
type="submit"
Expand Down
2 changes: 0 additions & 2 deletions src/core_plugins/timelion/public/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ app.controller('timelion', function (
$scope.safeSearch = _.debounce($scope.search, 500);

function saveSheet() {
savedSheet.id = savedSheet.title;
savedSheet.timelion_sheet = $scope.state.sheet;
savedSheet.timelion_interval = $scope.state.interval;
savedSheet.timelion_columns = $scope.state.columns;
Expand All @@ -239,7 +238,6 @@ app.controller('timelion', function (

function saveExpression(title) {
savedVisualizations.get({type: 'timelion'}).then(function (savedExpression) {
savedExpression.id = title;
savedExpression.visState.params = {
expression: $scope.state.sheet[$scope.state.selected],
interval: $scope.state.interval
Expand Down
6 changes: 3 additions & 3 deletions src/core_plugins/timelion/public/partials/save_sheet.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ <h4 class="list-group-item-heading">Save entire Timelion sheet</h4>
<label for="savedSheet" class="control-label">Save sheet as</label>
<input id="savedSheet" ng-model="opts.savedSheet.title" input-focus="select" class="form-control" placeholder="Name this sheet...">
</div>

<saved-object-save-as-check-box saved-object="opts.savedSheet"></saved-object-save-as-check-box>
<div class="form-group">
<button ng-disabled="!opts.savedSheet.title" type="submit" class="btn btn-primary">
Save
Expand Down Expand Up @@ -43,9 +45,7 @@ <h4 class="list-group-item-heading">Save current expression as Kibana dashboard
<input id="savedExpression" ng-model="panelTitle" input-focus="select" class="form-control" placeholder="Name this panel">
</div>
<div class="form-group">
<button ng-disabled="!panelTitle" type="submit" class="btn btn-primary">
Save
</button>
<button ng-disabled="!panelTitle" type="submit" class="btn btn-primary">Save</button>
</div>
</form>
</div>
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/autoload/modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,4 @@ import 'ui/typeahead';
import 'ui/url';
import 'ui/validate_date_interval';
import 'ui/watch_multi';
import 'ui/courier/saved_object/ui/saved_object_save_as_checkbox';
110 changes: 108 additions & 2 deletions src/ui/public/courier/__tests__/saved_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,22 @@
import ngMock from 'ng_mock';
import expect from 'expect.js';
import sinon from 'auto-release-sinon';

import BluebirdPromise from 'bluebird';

import SavedObjectFactory from '../saved_object/saved_object';
import { stubMapper } from 'test_utils/stub_mapper';
import IndexPatternFactory from 'ui/index_patterns/_index_pattern';
import DocSourceProvider from '../data_source/doc_source';

import { stubMapper } from 'test_utils/stub_mapper';


describe('Saved Object', function () {
require('test_utils/no_digest_promises').activateForSuite();

let SavedObject;
let IndexPattern;
let esStub;
let DocSource;

/**
* Some default es stubbing to avoid timeouts and allow a default type of 'dashboard'.
Expand All @@ -35,6 +39,7 @@ describe('Saved Object', function () {

// Necessary to avoid a timeout condition.
sinon.stub(esStub.indices, 'putMapping').returns(BluebirdPromise.resolve());
sinon.stub(esStub.indices, 'refresh').returns(BluebirdPromise.resolve());
}

/**
Expand Down Expand Up @@ -83,11 +88,112 @@ describe('Saved Object', function () {
SavedObject = Private(SavedObjectFactory);
IndexPattern = Private(IndexPatternFactory);
esStub = es;
DocSource = Private(DocSourceProvider);

mockEsService();
stubMapper(Private);
}));

describe('save', function () {
describe(' with copyOnSave', function () {
it('as true creates a copy on save success', function () {
const mockDocResponse = getMockedDocResponse('myId');
stubESResponse(mockDocResponse);
let newUniqueId;
return createInitializedSavedObject({type: 'dashboard', id: 'myId'}).then(savedObject => {
sinon.stub(DocSource.prototype, 'doIndex', function () {
newUniqueId = savedObject.id;
expect(newUniqueId).to.not.be('myId');
mockDocResponse._id = newUniqueId;
return BluebirdPromise.resolve(newUniqueId);
});
savedObject.copyOnSave = true;
return savedObject.save()
.then((id) => {
expect(id).to.be(newUniqueId);
});
});
});

it('as true does not create a copy when save fails', function () {
const mockDocResponse = getMockedDocResponse('myId');
stubESResponse(mockDocResponse);
let originalId = 'id1';
return createInitializedSavedObject({type: 'dashboard', id: originalId}).then(savedObject => {
sinon.stub(DocSource.prototype, 'doIndex', function () {
return BluebirdPromise.reject('simulated error');
});
savedObject.copyOnSave = true;
return savedObject.save().then(() => {
throw new Error('Expected a rejection');
}).catch(() => {
expect(savedObject.id).to.be(originalId);
});
});
});

it('as false does not create a copy', function () {
const mockDocResponse = getMockedDocResponse('myId');
stubESResponse(mockDocResponse);
const id = 'myId';
return createInitializedSavedObject({type: 'dashboard', id: id}).then(savedObject => {
sinon.stub(DocSource.prototype, 'doIndex', function () {
expect(savedObject.id).to.be(id);
return BluebirdPromise.resolve(id);
});
savedObject.copyOnSave = false;
return savedObject.save()
.then((id) => {
expect(id).to.be(id);
});
});
});
});

it('returns id from server on success', function () {
return createInitializedSavedObject({type: 'dashboard'}).then(savedObject => {
const mockDocResponse = getMockedDocResponse('myId');
stubESResponse(mockDocResponse);
return savedObject.save()
.then((id) => {
expect(id).to.be('myId');
});
});
});

describe('updates isSaving variable', function () {
it('on success', function () {
let id = 'id';
stubESResponse(getMockedDocResponse(id));
return createInitializedSavedObject({type: 'dashboard', id: id}).then(savedObject => {
sinon.stub(DocSource.prototype, 'doIndex', () => {
expect(savedObject.isSaving).to.be(true);
return BluebirdPromise.resolve(id);
});
expect(savedObject.isSaving).to.be(false);
return savedObject.save()
.then((id) => {
expect(savedObject.isSaving).to.be(false);
});
});
});

it('on failure', function () {
return createInitializedSavedObject({type: 'dashboard'}).then(savedObject => {
sinon.stub(DocSource.prototype, 'doIndex', () => {
expect(savedObject.isSaving).to.be(true);
return BluebirdPromise.reject();
});
expect(savedObject.isSaving).to.be(false);
return savedObject.save()
.catch(() => {
expect(savedObject.isSaving).to.be(false);
});
});
});
});
});

describe('applyESResp', function () {
it('throws error if not found', function () {
return createInitializedSavedObject({ type: 'dashboard' }).then(savedObject => {
Expand Down
Loading