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

Allow plugins to hide write controls in the dashboard app #11537

Merged
merged 14 commits into from
May 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 4 additions & 21 deletions src/core_plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import * as systemApi from './server/lib/system_api';
import handleEsError from './server/lib/handle_es_error';
import mappings from './mappings.json';

import { injectVars } from './inject_vars';

const mkdirp = Promise.promisify(mkdirpNode);

module.exports = function (kibana) {
Expand Down Expand Up @@ -43,27 +45,7 @@ module.exports = function (kibana) {
'devTools',
'docViews'
],
injectVars: function (server) {
const serverConfig = server.config();

//DEPRECATED SETTINGS
//if the url is set, the old settings must be used.
//keeping this logic for backward compatibilty.
const configuredUrl = server.config().get('tilemap.url');
const isOverridden = typeof configuredUrl === 'string' && configuredUrl !== '';
const tilemapConfig = serverConfig.get('tilemap');

return {
kbnDefaultAppId: serverConfig.get('kibana.defaultAppId'),
tilemapsConfig: {
deprecated: {
isOverridden: isOverridden,
config: tilemapConfig,
},
manifestServiceUrl: serverConfig.get('tilemap.manifestServiceUrl')
}
};
},
injectVars,
},

links: [
Expand Down Expand Up @@ -148,6 +130,7 @@ module.exports = function (kibana) {

server.expose('systemApi', systemApi);
server.expose('handleEsError', handleEsError);
server.expose('injectVars', injectVars);
}
});
};
21 changes: 21 additions & 0 deletions src/core_plugins/kibana/inject_vars.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export function injectVars(server) {
const serverConfig = server.config();

//DEPRECATED SETTINGS
//if the url is set, the old settings must be used.
//keeping this logic for backward compatibilty.
const configuredUrl = server.config().get('tilemap.url');
const isOverridden = typeof configuredUrl === 'string' && configuredUrl !== '';
const tilemapConfig = serverConfig.get('tilemap');

return {
kbnDefaultAppId: serverConfig.get('kibana.defaultAppId'),
tilemapsConfig: {
deprecated: {
isOverridden,
config: tilemapConfig,
},
manifestServiceUrl: serverConfig.get('tilemap.manifestServiceUrl')
}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ describe('DashboardState', function () {
let SavedDashboard;
let timefilter;
let quickTimeRanges;
let dashboardConfig;

function initDashboardState() {
dashboardState = new DashboardState(savedDashboard, AppState);
dashboardState = new DashboardState(savedDashboard, AppState, dashboardConfig);
}

beforeEach(ngMock.module('kibana'));
Expand All @@ -21,6 +22,7 @@ describe('DashboardState', function () {
quickTimeRanges = $injector.get('quickRanges');
AppState = $injector.get('AppState');
SavedDashboard = $injector.get('SavedDashboard');
dashboardConfig = $injector.get('dashboardConfig');
savedDashboard = new SavedDashboard();
}));

Expand Down
19 changes: 13 additions & 6 deletions src/core_plugins/kibana/public/dashboard/dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,13 @@ app.directive('dashboardApp', function ($injector) {
const kbnUrl = $injector.get('kbnUrl');
const confirmModal = $injector.get('confirmModal');
const Private = $injector.get('Private');

const brushEvent = Private(UtilsBrushEventProvider);
const filterBarClickHandler = Private(FilterBarClickHandlerProvider);

return {
restrict: 'E',
controllerAs: 'dashboardApp',
controller: function ($scope, $rootScope, $route, $routeParams, $location, getAppState, $compile) {
controller: function ($scope, $rootScope, $route, $routeParams, $location, getAppState, $compile, dashboardConfig) {
const filterBar = Private(FilterBarQueryFilterProvider);
const docTitle = Private(DocTitleProvider);
const notify = new Notifier({ location: 'Dashboard' });
Expand All @@ -98,7 +97,7 @@ app.directive('dashboardApp', function ($injector) {
docTitle.change(dash.title);
}

const dashboardState = new DashboardState(dash, AppState);
const dashboardState = new DashboardState(dash, AppState, dashboardConfig);

// The 'previouslyStored' check is so we only update the time filter on dashboard open, not during
// normal cross app navigation.
Expand Down Expand Up @@ -151,8 +150,16 @@ app.directive('dashboardApp', function ($injector) {
dashboardState.getIsDirty(timefilter));
$scope.newDashboard = () => { kbnUrl.change(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {}); };
$scope.saveState = () => dashboardState.saveState();
$scope.getShouldShowEditHelp = () => !dashboardState.getPanels().length && dashboardState.getIsEditMode();
$scope.getShouldShowViewHelp = () => !dashboardState.getPanels().length && dashboardState.getIsViewMode();
$scope.getShouldShowEditHelp = () => (
!dashboardState.getPanels().length &&
dashboardState.getIsEditMode() &&
!dashboardConfig.getHideWriteControls()
);
$scope.getShouldShowViewHelp = () => (
!dashboardState.getPanels().length &&
dashboardState.getIsViewMode() &&
!dashboardConfig.getHideWriteControls()
);

$scope.toggleExpandPanel = (panelIndex) => {
if ($scope.expandedPanel && $scope.expandedPanel.panelIndex === panelIndex) {
Expand Down Expand Up @@ -207,7 +214,7 @@ app.directive('dashboardApp', function ($injector) {
$scope.$listen(timefilter, 'fetch', $scope.refresh);

function updateViewMode(newMode) {
$scope.topNavMenu = getTopNavConfig(newMode, navActions); // eslint-disable-line no-use-before-define
$scope.topNavMenu = dashboardConfig.getHideWriteControls() ? [] : getTopNavConfig(newMode, navActions); // eslint-disable-line no-use-before-define
dashboardState.switchViewMode(newMode);
$scope.dashboardViewMode = newMode;
}
Expand Down
22 changes: 22 additions & 0 deletions src/core_plugins/kibana/public/dashboard/dashboard_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { uiModules } from 'ui/modules';
uiModules.get('kibana')
.provider('dashboardConfig', () => {
let hideWriteControls = false;

return {
/**
* Part of the exposed plugin API - do not remove without careful consideration.
* @type {boolean}
*/
turnHideWriteControlsOn() {
hideWriteControls = true;
},
$get() {
return {
getHideWriteControls() {
Copy link
Contributor

Choose a reason for hiding this comment

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

More of a general question, for getters that return booleans, should the getter be named shouldHide... or canHide...? It seems more expressive when calling it within conditionals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the get prefix because I think it makes it clear that it's not a setter. e.g. you can't immediately tell if shouldHide.. would take a boolean and set the value of shouldHide, or is getting the shouldHide variable. Though I agree it reads nicer when it's if (shouldHide)....

I suppose I wouldn't be opposed to calling it getShouldHideWriteControls, but I'm not sure the should really adds anything.

I'd prefer not to hold up the PR on this but it might be worthwhile to create another issue to come to a team conclusion on the best way to name getters and then add it to the style guide. I've had reviews in the past with @cjcenizal that touch on this issue as well, and I don't think we ever settled on something concrete.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, my opinion is that can and should are pretty ambiguous because whether something "can" be or "should" be is pretty relative and open to interpretation. getIs also feels a little strange to me when I read it. I generally just stick with is and are where possible, e.g. areWriteControlsVisible or writeControls.isVisible. But I don't think we have an agreed-upon convention, so until we have that I think the most efficient way forward is if we each just write names in ways that we're comfortable with and, to Stacey's point, not get too hung up on nitpicking the way other people write them.

return hideWriteControls;
}
};
}
};
});
14 changes: 8 additions & 6 deletions src/core_plugins/kibana/public/dashboard/dashboard_state.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { stateMonitorFactory } from 'ui/state_management/state_monitor_factory';
import { createPanelState } from 'plugins/kibana/dashboard/panel/panel_state';
import { getPersistedStateId } from 'plugins/kibana/dashboard/panel/panel_state';

function getStateDefaults(dashboard) {
function getStateDefaults(dashboard, hideWriteControls) {
return {
title: dashboard.title,
description: dashboard.description,
Expand All @@ -19,7 +19,7 @@ function getStateDefaults(dashboard) {
uiState: dashboard.uiStateJSON ? JSON.parse(dashboard.uiStateJSON) : {},
query: FilterUtils.getQueryFilterForDashboard(dashboard),
filters: FilterUtils.getFilterBarsForDashboard(dashboard),
viewMode: dashboard.id ? DashboardViewMode.VIEW : DashboardViewMode.EDIT,
viewMode: dashboard.id || hideWriteControls ? DashboardViewMode.VIEW : DashboardViewMode.EDIT,
};
}

Expand Down Expand Up @@ -59,11 +59,13 @@ export class DashboardState {
*
* @param savedDashboard {SavedDashboard}
* @param AppState {AppState}
* @param dashboardConfig {DashboardConfigProvider}
*/
constructor(savedDashboard, AppState) {
constructor(savedDashboard, AppState, dashboardConfig) {
this.savedDashboard = savedDashboard;
this.dashboardConfig = dashboardConfig;

this.stateDefaults = getStateDefaults(this.savedDashboard);
this.stateDefaults = getStateDefaults(this.savedDashboard, this.dashboardConfig.getHideWriteControls());

this.appState = new AppState(this.stateDefaults);
this.uiState = this.appState.makeStateful('uiState');
Expand Down Expand Up @@ -93,7 +95,7 @@ export class DashboardState {
// The right way to fix this might be to ensure the defaults object stored on state is a deep
// clone, but given how much code uses the state object, I determined that to be too risky of a change for
// now. TODO: revisit this!
this.stateDefaults = getStateDefaults(this.savedDashboard);
this.stateDefaults = getStateDefaults(this.savedDashboard, this.dashboardConfig.getHideWriteControls());
// The original query won't be restored by the above because the query on this.savedDashboard is applied
// in place in order for it to affect the visualizations.
this.stateDefaults.query = this.lastSavedDashboardFilters.query;
Expand Down Expand Up @@ -223,7 +225,7 @@ export class DashboardState {
* @returns {DashboardViewMode}
*/
getViewMode() {
return this.appState.viewMode;
return this.dashboardConfig.getHideWriteControls() ? DashboardViewMode.VIEW : this.appState.viewMode;
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kibana/public/dashboard/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'plugins/kibana/dashboard/dashboard';
import 'plugins/kibana/dashboard/saved_dashboard/saved_dashboards';
import 'plugins/kibana/dashboard/styles/index.less';
import 'plugins/kibana/dashboard/dashboard_config';
import uiRoutes from 'ui/routes';
import { SavedObjectRegistryProvider } from 'ui/saved_objects/saved_object_registry';
import { savedDashboardRegister } from 'plugins/kibana/dashboard/saved_dashboard/saved_dashboard_register';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
class="kuiButton kuiButton--danger"
ng-click="listingController.deleteSelectedItems()"
aria-label="Delete selected dashboards"
ng-if="listingController.getSelectedItemsCount() > 0"
ng-if="listingController.getSelectedItemsCount() > 0 && !listingController.hideWriteControls"
tooltip="Delete selected dashboards"
tooltip-append-to-body="true"
data-test-subj="deleteSelectedDashboards"
Expand All @@ -63,7 +63,7 @@
href="{{listingController.getCreateDashboardHref()}}"
aria-label="Create new dashboard"
data-test-subj="newDashboardLink"
ng-if="listingController.getSelectedItemsCount() === 0"
ng-if="listingController.getSelectedItemsCount() === 0 && !listingController.hideWriteControls"
tooltip="Create new dashboard"
tooltip-append-to-body="true"
>
Expand Down Expand Up @@ -104,10 +104,10 @@
>
<div class="kuiPromptForItems">
<div class="kuiPromptForItems__message">
Looks like you don&rsquo;t have any dashboards. Let&rsquo;s create some!
Looks like you don&rsquo;t have any dashboards. <span ng-if="!listingController.hideWriteControls">Let&rsquo;s create some!</span>
</div>

<div class="kuiPromptForItems__actions">
<div class="kuiPromptForItems__actions" ng-if="!listingController.hideWriteControls">
<a
class="kuiButton kuiButton--primary kuiButton--iconText"
data-test-subj="createDashboardPromptButton"
Expand All @@ -126,7 +126,7 @@
<table class="kuiTable" ng-if="listingController.items.length">
<thead>
<tr>
<th class="kuiTableHeaderCell kuiTableHeaderCell--checkBox">
<th class="kuiTableHeaderCell kuiTableHeaderCell--checkBox" ng-if="!listingController.hideWriteControls">
<input
type="checkbox"
class="kuiCheckBox"
Expand Down Expand Up @@ -161,7 +161,7 @@
ng-repeat="item in listingController.pageOfItems track by item.id"
class="kuiTableRow"
>
<td class="kuiTableRowCell kuiTableRowCell--checkBox">
<td class="kuiTableRowCell kuiTableRowCell--checkBox" ng-if="!listingController.hideWriteControls">
<div class="kuiTableRowCell__liner">
<input
type="checkbox"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export function DashboardListingController($injector, $scope) {
const Private = $injector.get('Private');
const timefilter = $injector.get('timefilter');
const config = $injector.get('config');
const dashboardConfig = $injector.get('dashboardConfig');

timefilter.enabled = false;

Expand Down Expand Up @@ -72,6 +73,8 @@ export function DashboardListingController($injector, $scope) {

this.pager = pagerFactory.create(this.items.length, 20, 1);

this.hideWriteControls = dashboardConfig.getHideWriteControls();

$scope.$watch(() => this.filter, () => {
deselectAll();
fetchItems();
Expand Down
2 changes: 2 additions & 0 deletions src/core_plugins/kibana/public/dashboard/panel/panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import 'ui/visualize';
import 'ui/doc_table';
import * as columnActions from 'ui/doc_table/actions/columns';
import 'plugins/kibana/dashboard/panel/get_object_loaders_for_dashboard';
import 'plugins/kibana/visualize/saved_visualizations';
import 'plugins/kibana/discover/saved_searches';
import { FilterManagerProvider } from 'ui/filter_manager';
import { uiModules } from 'ui/modules';
import panelTemplate from 'plugins/kibana/dashboard/panel/panel.html';
Expand Down
22 changes: 2 additions & 20 deletions src/core_plugins/kibana/public/kibana.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// autoloading

// preloading (for faster webpack builds)
import moment from 'moment-timezone';
import chrome from 'ui/chrome';
import routes from 'ui/routes';
import { uiModules } from 'ui/modules';
Expand All @@ -21,6 +20,7 @@ import 'ui/timepicker';
import 'ui/react_components';
import { Notifier } from 'ui/notify/notifier';
import 'leaflet';
import { KibanaRootController } from './kibana_root_controller';

routes.enable();

Expand All @@ -29,24 +29,6 @@ routes
redirectTo: `/${chrome.getInjected('kbnDefaultAppId', 'discover')}`
});

chrome
.setRootController('kibana', function ($scope, courier, config) {
// wait for the application to finish loading
$scope.$on('application.load', function () {
courier.start();
});

config.watch('dateFormat:tz', setDefaultTimezone, $scope);
config.watch('dateFormat:dow', setStartDayOfWeek, $scope);

function setDefaultTimezone(tz) {
moment.tz.setDefault(tz);
}

function setStartDayOfWeek(day) {
const dow = moment.weekdays().indexOf(day);
moment.updateLocale(moment.locale(), { week: { dow } });
}
});
chrome.setRootController('kibana', KibanaRootController);

uiModules.get('kibana').run(Notifier.pullMessageFromUrl);
20 changes: 20 additions & 0 deletions src/core_plugins/kibana/public/kibana_root_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import moment from 'moment-timezone';

export function KibanaRootController($scope, courier, config) {
// wait for the application to finish loading
$scope.$on('application.load', function () {
courier.start();
});

config.watch('dateFormat:tz', setDefaultTimezone, $scope);
config.watch('dateFormat:dow', setStartDayOfWeek, $scope);

function setDefaultTimezone(tz) {
moment.tz.setDefault(tz);
}

function setStartDayOfWeek(day) {
const dow = moment.weekdays().indexOf(day);
moment.updateLocale(moment.locale(), { week: { dow } });
}
}
5 changes: 5 additions & 0 deletions src/ui/public/chrome/api/nav.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { remove } from 'lodash';
import { parse, format } from 'url';
import { isString } from 'lodash';

Expand All @@ -18,6 +19,10 @@ export function initChromeNavApi(chrome, internals) {
return navLink;
};

chrome.showOnlyById = (id) => {
remove(internals.nav, app => app.id !== id);
};

chrome.getBasePath = function () {
return internals.basePath || '';
};
Expand Down
13 changes: 10 additions & 3 deletions test/functional/page_objects/dashboard_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,16 @@ export function DashboardPageProvider({ getService, getPageObjects }) {
async loadSavedDashboard(dashName) {
log.debug(`Load Saved Dashboard ${dashName}`);

await this.searchForDashboardWithName(dashName);
await this.clickDashboardByLinkText(dashName);
return PageObjects.header.waitUntilLoadingHasFinished();
await retry.try(async () => {
await this.searchForDashboardWithName(dashName);
await this.clickDashboardByLinkText(dashName);
await PageObjects.header.waitUntilLoadingHasFinished();

const onDashboardLandingPage = await this.onDashboardLandingPage();
if (onDashboardLandingPage) {
throw new Error('Failed to open the dashboard up');
}
});
}

getPanelTitles() {
Expand Down
Loading