Introduce CodeOwnersModel and ModelLoader and use them in UI elements
This change removes direct dependencies between CodeOwnersService
and UI elements. Instead, UI elements displays/uses model as a
data source and calls ModelLoader when a model property should be
populated with a data.
This change doesn't update suggest-owners.js (it displays the
list of suggested owners). The suggest-owners.js is updated in the
upcoming change.
Change-Id: I9d4c4fda6eac4aab674a9928db9f1c408f93b8f3
diff --git a/.eslintrc.json b/.eslintrc.json
index b38415b..2c0d24a 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -101,7 +101,8 @@
"Polymer",
"LegacyElementMixin",
"GestureEventListeners",
- "LegacyDataMixin"
+ "LegacyDataMixin",
+ "CodeOwnersModelMixin"
]
}
],
diff --git a/ui/code-owners-model-loader.js b/ui/code-owners-model-loader.js
new file mode 100644
index 0000000..ea10813
--- /dev/null
+++ b/ui/code-owners-model-loader.js
@@ -0,0 +1,78 @@
+/**
+ * @license
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * ModelLoader provides a method for loading data into the model.
+ * It is a bridge between an ownersModel and an ownersService.
+ * When UI elements depends on a model property XXX, the element should
+ * observe (or bind to) the property and call modelLoader.loadXXX method
+ * to load the value.
+ * It is recommended to use CodeOwnersModelMixin and call all load... methods
+ * in the loadPropertiesAfterModelChanged method
+ */
+export class ModelLoader {
+ constructor(ownersService, ownersModel) {
+ this.ownersService = ownersService;
+ this.ownersModel = ownersModel;
+ }
+
+ async _loadProperty(propertyName, propertyLoader, propertySetter) {
+ // Load property only if it was not already loaded
+ if (this.ownersModel[propertyName] !== undefined) return;
+ const newValue = await propertyLoader();
+ // It is possible, that several requests is made in parallel.
+ // Store only the first result and discard all other results.
+ // (also, due to the CodeOwnersCacheApi all result must be identical)
+ if (this.ownersModel[propertyName] !== undefined) return;
+ propertySetter(newValue);
+ }
+
+ async loadBranchConfig() {
+ await this._loadProperty('branchConfig',
+ () => this.ownersService.getBranchConfig(),
+ value => this.ownersModel.setBranchConfig(value)
+ );
+ }
+
+ async loadStatus() {
+ await this._loadProperty('status',
+ () => this.ownersService.getStatus(),
+ value => this.ownersModel.setStatus(value)
+ );
+ }
+
+ async loadUserRole() {
+ await this._loadProperty('userRole',
+ () => this.ownersService.getLoggedInUserInitialRole(),
+ value => this.ownersModel.setUserRole(value)
+ );
+ }
+
+ async loadIsCodeOwnerEnabled() {
+ await this._loadProperty('isCodeOwnerEnabled',
+ () => this.ownersService.isCodeOwnerEnabled(),
+ value => this.ownersModel.setIsCodeOwnerEnabled(value)
+ );
+ }
+
+ async loadAreAllFilesApproved() {
+ await this._loadProperty('areAllFilesApproved',
+ () => this.ownersService.areAllFilesApproved(),
+ value => this.ownersModel.setAreAllFilesApproved(value)
+ );
+ }
+}
diff --git a/ui/code-owners-model-mixin.js b/ui/code-owners-model-mixin.js
new file mode 100644
index 0000000..722af6f
--- /dev/null
+++ b/ui/code-owners-model-mixin.js
@@ -0,0 +1,100 @@
+/**
+ * @license
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import {CodeOwnerService} from './code-owners-service.js';
+import {ModelLoader} from './code-owners-model-loader.js';
+import {CodeOwnersModel} from './code-owners-model.js';
+
+/**
+ * The CodeOwnersMixin adds several properties to a class and translates
+ * 'model-property-changed' events from the model to the notifyPath calls.
+ * This allows to use properties of the model in observers, calculated
+ * properties and bindings.
+ * It is guaranteed, that model and modelLoader are set only if change,
+ * reporting and restApi properties are set.
+ */
+export const CodeOwnersModelMixin = Polymer.dedupingMixin(base => {
+ return class extends base {
+ constructor(...args) {
+ super(...args);
+ /**
+ * The modelLoader allows an element to request a property
+ * Typically should be used in loadPropertiesAfterModelChanged
+ * to ensure that all required model properties are loaded
+ */
+ this.modelLoader = undefined;
+ }
+ static get properties() {
+ return {
+ /* The following 3 properties (change, reporting, restApi) have to be
+ * set from the outside for the mixin to work.
+ */
+ change: Object,
+ reporting: Object,
+ restApi: Object,
+ model: {
+ type: Object,
+ observer: '_modelChanged',
+ },
+ };
+ }
+
+ static get observers() {
+ return ['onInputChanged(restApi, change, reporting)'];
+ }
+
+ onInputChanged(restApi, change, reporting) {
+ if ([restApi, change, reporting].includes(undefined)) {
+ this.model = undefined;
+ this.modelLoader = undefined;
+ return;
+ }
+ const ownerService = CodeOwnerService.getOwnerService(
+ this.restApi,
+ this.change
+ );
+ const model = CodeOwnersModel.getModel(change);
+ this.modelLoader = new ModelLoader(ownerService, model);
+ // Assign model after modelLoader, so modelLoader can be used in
+ // the _requirePropertiesAfterModelChanged method
+ this.model = model;
+ }
+
+ _modelChanged(newModel) {
+ if (this.modelPropertyChangedUnsubscriber) {
+ this.modelPropertyChangedUnsubscriber();
+ this.modelPropertyChangedUnsubscriber = undefined;
+ }
+ if (!newModel) return;
+ const propertyChangedListener = e => {
+ this.notifyPath(`model.${e.detail.propertyName}`);
+ };
+ newModel.addEventListener('model-property-changed',
+ propertyChangedListener);
+ this.modelPropertyChangedUnsubscriber = () => {
+ newModel.removeEventListener('model-property-changed',
+ propertyChangedListener);
+ };
+ this.loadPropertiesAfterModelChanged();
+ }
+
+ loadPropertiesAfterModelChanged() {
+ // The class should override this method and calls appropriate methods
+ // from this.modelLoader to ensure that all required properties are
+ // set in model.
+ }
+ };
+});
diff --git a/ui/code-owners-model.js b/ui/code-owners-model.js
new file mode 100644
index 0000000..5600773
--- /dev/null
+++ b/ui/code-owners-model.js
@@ -0,0 +1,90 @@
+/**
+ * @license
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Maintain the state of code-owners.
+ * Raises 'model-property-changed' event when a property is changed.
+ * The plugin shares the same model between all UI elements (thought it is not
+ * required).
+ * UI elements use values from this model to display information
+ * and listens for the model-property-changed event. To do so, UI elements
+ * add CodeOwnersModelMixin, which is doing the listening and the translation
+ * from model-property-changed event to Polymer property-changed-event. The
+ * translation allows to use model properties in observables, bindings,
+ * computed properties, etc...
+ * The CodeOwnersModelLoader updates the model.
+ *
+ * It would be good to use RxJs Observable for implementing model properties.
+ * However, RxJs library is imported by Gerrit and there is no
+ * good way to reuse the same library in the plugin.
+ */
+export class CodeOwnersModel extends EventTarget {
+ constructor(change) {
+ super();
+ this.change = change;
+ this.branchConfig = undefined;
+ this.status = undefined;
+ this.userRole = undefined;
+ this.isCodeOwnerEnabled = undefined;
+ this.areAllFilesApproved = undefined;
+ }
+
+ setBranchConfig(config) {
+ if (this.branchConfig === config) return;
+ this.branchConfig = config;
+ this._firePropertyChanged('branchConfig');
+ }
+
+ setStatus(status) {
+ if (this.status === status) return;
+ this.status = status;
+ this._firePropertyChanged('status');
+ }
+
+ setUserRole(userRole) {
+ if (this.userRole === userRole) return;
+ this.userRole = userRole;
+ this._firePropertyChanged('userRole');
+ }
+
+ setIsCodeOwnerEnabled(enabled) {
+ if (this.isCodeOwnerEnabled === enabled) return;
+ this.isCodeOwnerEnabled = enabled;
+ this._firePropertyChanged('isCodeOwnerEnabled');
+ }
+
+ setAreAllFilesApproved(approved) {
+ if (this.areAllFilesApproved === approved) return;
+ this.areAllFilesApproved = approved;
+ this._firePropertyChanged('areAllFilesApproved');
+ }
+
+ _firePropertyChanged(propertyName) {
+ this.dispatchEvent(new CustomEvent('model-property-changed', {
+ detail: {
+ propertyName,
+ },
+ }));
+ }
+
+ static getModel(change) {
+ if (!this.model || this.model.change !== change) {
+ this.model = new CodeOwnersModel(change);
+ }
+ return this.model;
+ }
+}
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js
index 9f29d6d..52638ac 100644
--- a/ui/code-owners-service.js
+++ b/ui/code-owners-service.js
@@ -546,7 +546,7 @@
this.codeOwnerCacheApi = new CodeOwnersCacheApi(codeOwnerApi, change);
}
- getBranchConfig() {
+ async getBranchConfig() {
return this.codeOwnerCacheApi.getBranchConfig();
}
@@ -570,3 +570,4 @@
return this.ownerService;
}
}
+
diff --git a/ui/owner-requirement.js b/ui/owner-requirement.js
index 44d14ca..b7b9dbb 100644
--- a/ui/owner-requirement.js
+++ b/ui/owner-requirement.js
@@ -15,8 +15,9 @@
* limitations under the License.
*/
-import {CodeOwnerService, OwnerStatus} from './code-owners-service.js';
+import {OwnerStatus} from './code-owners-service.js';
import {ownerState} from './owner-ui-state.js';
+import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
/**
* Owner requirement control for `submit-requirement-item-code-owners` endpoint.
@@ -24,7 +25,8 @@
* This will show the status and suggest owners button next to
* the code-owners submit requirement.
*/
-export class OwnerRequirementValue extends Polymer.Element {
+export class OwnerRequirementValue extends
+ CodeOwnersModelMixin(Polymer.Element) {
static get is() {
return 'owner-requirement-value';
}
@@ -80,83 +82,82 @@
static get properties() {
return {
- change: Object,
- reporting: Object,
- restApi: Object,
-
- ownerService: Object,
-
_statusCount: Object,
_isLoading: {
type: Boolean,
- value: true,
+ computed: '_computeIsLoading(model.branchConfig, model.status, '
+ + 'model.userRole)',
},
_allApproved: {
type: Boolean,
computed: '_computeAllApproved(_statusCount)',
},
- _isOverriden: Boolean,
- _overrideInfoUrl: String,
+ _isOverriden: {
+ type: Boolean,
+ computed: '_computeIsOverriden(model.branchConfig)',
+ },
+ _overrideInfoUrl: {
+ type: String,
+ computed: '_computeOverrideInfoUrl(model.branchConfig)',
+ },
};
}
static get observers() {
return [
- 'onInputChanged(restApi, change, reporting)',
+ '_onStatusChanged(model.status, model.userRole)',
];
}
- _checkIfOverriden() {
- this.ownerService.getBranchConfig().then(res => {
- if (!res['override_approval']) {
- // no override label configured
- this._isOverriden = false;
- return;
- }
-
- const overridenLabel = res['override_approval'].label;
- const overridenValue = res['override_approval'].value;
-
- if (this.change.labels[overridenLabel]) {
- const votes = this.change.labels[overridenLabel].all || [];
- if (votes.find(v => `${v.value}` === `${overridenValue}`)) {
- this._isOverriden = true;
- return;
- }
- }
-
- // otherwise always reset it to false
- this._isOverriden = false;
- });
- }
-
- _updateStatus() {
- this._isLoading = true;
+ loadPropertiesAfterModelChanged() {
+ super.loadPropertiesAfterModelChanged();
this.reporting.reportLifeCycle('owners-submit-requirement-summary-start');
-
- return this.ownerService.getStatus()
- .then(({rawStatuses}) => {
- this._statusCount = this._getStatusCount(rawStatuses);
- this.ownerService.getLoggedInUserInitialRole().then(role => {
- // Send a metric with overall summary when code owners submit
- // requirement shown and finished fetching status
- this.reporting.reportLifeCycle(
- 'owners-submit-requirement-summary-shown',
- {...this._statusCount, user_role: role}
- );
- });
- })
- .finally(() => {
- this._isLoading = false;
- });
+ this.modelLoader.loadBranchConfig();
+ this.modelLoader.loadStatus();
+ this.modelLoader.loadUserRole();
}
- _updateOverrideInfoUrl() {
- this.ownerService.getBranchConfig().then(config => {
- this._overrideInfoUrl = config.general && config.general.override_info_url
- ?
- config.general.override_info_url : '';
- });
+ _computeIsLoading(branchConfig, status, userRole) {
+ return !branchConfig || !status || !userRole;
+ }
+
+ _onStatusChanged(status, userRole) {
+ if (!status || !userRole) {
+ this._statusCount = undefined;
+ return;
+ }
+ const rawStatuses = status.rawStatuses;
+ this._statusCount = this._getStatusCount(rawStatuses);
+ this.reporting.reportLifeCycle('owners-submit-requirement-summary-shown',
+ {...this._statusCount, user_role: userRole});
+ }
+
+ _computeOverrideInfoUrl(branchConfig) {
+ if (!branchConfig) {
+ return '';
+ }
+ return branchConfig.general && branchConfig.general.override_info_url
+ ? branchConfig.general.override_info_url : '';
+ }
+
+ _computeIsOverriden(branchConfig) {
+ if (!branchConfig || !branchConfig['override_approval']) {
+ // no override label configured
+ return false;
+ }
+
+ const overridenLabel = branchConfig['override_approval'].label;
+ const overridenValue = branchConfig['override_approval'].value;
+
+ if (this.change.labels[overridenLabel]) {
+ const votes = this.change.labels[overridenLabel].all || [];
+ if (votes.find(v => `${v.value}` === `${overridenValue}`)) {
+ return true;
+ }
+ }
+
+ // otherwise always reset it to false
+ return false;
}
_computeAllApproved(statusCount) {
@@ -188,6 +189,7 @@
}
_computeStatusText(statusCount, isOverriden) {
+ if (statusCount === undefined || isOverriden === undefined) return '';
const statusText = [];
if (statusCount.missing) {
statusText.push(`${statusCount.missing} missing`);
@@ -212,14 +214,6 @@
return status === OwnerStatus.PENDING;
}
- onInputChanged(restApi, change, reporting) {
- if ([restApi, change, reporting].includes(undefined)) return;
- this.ownerService = CodeOwnerService.getOwnerService(this.restApi, change);
- this._updateStatus();
- this._checkIfOverriden();
- this._updateOverrideInfoUrl();
- }
-
_openReplyDialog() {
this.dispatchEvent(
new CustomEvent('open-reply-dialog', {
@@ -229,10 +223,8 @@
})
);
ownerState.expandSuggestion = true;
- this.ownerService.getLoggedInUserInitialRole().then(role => {
- this.reporting.reportInteraction(
- 'suggest-owners-from-submit-requirement', {user_role: role});
- });
+ this.reporting.reportInteraction('suggest-owners-from-submit-requirement',
+ {user_role: this.model.userRole});
}
}
diff --git a/ui/owner-status-column.js b/ui/owner-status-column.js
index a63ef8b..31ad654 100644
--- a/ui/owner-status-column.js
+++ b/ui/owner-status-column.js
@@ -15,7 +15,8 @@
* limitations under the License.
*/
-import {CodeOwnerService, OwnerStatus} from './code-owners-service.js';
+import {OwnerStatus} from './code-owners-service.js';
+import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
const MAGIC_FILES = ['/COMMIT_MSG', '/MERGE_LIST', '/PATCHSET_LEVEL'];
const STATUS_CODE = {
@@ -46,7 +47,7 @@
[STATUS_CODE.ERROR]: 'Failed to fetch code owner status',
};
-class BaseEl extends Polymer.Element {
+class BaseEl extends CodeOwnersModelMixin(Polymer.Element) {
computeHidden(change, patchRange) {
if ([change, patchRange].includes(undefined)) return true;
// if code-owners is not a submit requirement, don't show status column
@@ -66,11 +67,6 @@
if (`${patchRange.patchNum}` !== `${latestPatchset._number}`) return true;
return false;
}
-
- onInputChanged(restApi, change) {
- if ([restApi, change].includes(undefined)) return;
- this.ownerService = CodeOwnerService.getOwnerService(this.restApi, change);
- }
}
/**
@@ -96,26 +92,15 @@
static get properties() {
return {
- change: Object,
- reporting: Object,
patchRange: Object,
- restApi: Object,
hidden: {
type: Boolean,
reflectToAttribute: true,
computed: 'computeHidden(change, patchRange)',
},
- ownerService: Object,
};
}
-
- static get observers() {
- return [
- 'onInputChanged(restApi, change)',
- 'onOwnerServiceChanged(ownerServive)',
- ];
- }
}
customElements.define(OwnerStatusColumnHeader.is, OwnerStatusColumnHeader);
@@ -130,10 +115,6 @@
static get properties() {
return {
- change: Object,
- reporting: Object,
- restApi: Object,
-
path: String,
patchRange: Object,
hidden: {
@@ -188,47 +169,45 @@
static get observers() {
return [
- 'onInputChanged(restApi, change)',
- 'computeStatusIcon(ownerService, path)',
+ 'computeStatusIcon(model.status, path)',
];
}
- computeStatusIcon(ownerService, path) {
- if ([ownerService, path].includes(undefined)) return;
+ loadPropertiesAfterModelChanged() {
+ super.loadPropertiesAfterModelChanged();
+ this.modelLoader.loadStatus();
+ }
+
+ computeStatusIcon(modelStatus, path) {
+ if ([modelStatus, path].includes(undefined)) return;
if (MAGIC_FILES.includes(path)) return;
- ownerService.getStatus()
- .then(({codeOwnerStatusMap}) => {
- const statusItem = codeOwnerStatusMap.get(path);
- if (!statusItem) {
- this.status = STATUS_CODE.ERROR;
- return;
- }
+ const codeOwnerStatusMap = modelStatus.codeOwnerStatusMap;
+ const statusItem = codeOwnerStatusMap.get(path);
+ if (!statusItem) {
+ this.status = STATUS_CODE.ERROR;
+ return;
+ }
- const status = statusItem.status;
- let oldPathStatus = null;
- if (statusItem.oldPath) {
- const oldStatusItem = codeOwnerStatusMap.get(statusItem.oldPath);
- if (!oldStatusItem) {
- // should not happen
- } else {
- oldPathStatus = oldStatusItem.status;
- }
- }
+ const status = statusItem.status;
+ let oldPathStatus = null;
+ if (statusItem.oldPath) {
+ const oldStatusItem = codeOwnerStatusMap.get(statusItem.oldPath);
+ if (!oldStatusItem) {
+ // should not happen
+ } else {
+ oldPathStatus = oldStatusItem.status;
+ }
+ }
- const newPathStatus = this._computeStatus(status);
- if (!oldPathStatus) {
- this.status = newPathStatus;
- } else {
- this.status = newPathStatus === STATUS_CODE.APPROVED
- ? this._computeStatus(oldPathStatus, /* oldPath= */ true)
- : newPathStatus;
- }
- })
- .catch(e => {
- this.status = STATUS_CODE.ERROR;
- throw e;
- });
+ const newPathStatus = this._computeStatus(status);
+ if (!oldPathStatus) {
+ this.status = newPathStatus;
+ } else {
+ this.status = newPathStatus === STATUS_CODE.APPROVED
+ ? this._computeStatus(oldPathStatus, /* oldPath= */ true)
+ : newPathStatus;
+ }
}
_computeIcon(status) {
diff --git a/ui/suggest-owners-trigger.js b/ui/suggest-owners-trigger.js
index 8e799f4..c6fd55d 100644
--- a/ui/suggest-owners-trigger.js
+++ b/ui/suggest-owners-trigger.js
@@ -14,10 +14,11 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {CodeOwnerService} from './code-owners-service.js';
import {ownerState} from './owner-ui-state.js';
+import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
-export class SuggestOwnersTrigger extends Polymer.Element {
+export class SuggestOwnersTrigger extends
+ CodeOwnersModelMixin(Polymer.Element) {
static get is() {
return 'suggest-owners-trigger';
}
@@ -29,26 +30,19 @@
static get properties() {
return {
- change: Object,
- reporting: Object,
- restApi: Object,
-
expanded: {
type: Boolean,
value: false,
},
hidden: {
type: Boolean,
- value: true,
+ computed: '_computeHidden(model.isCodeOwnerEnabled,' +
+ 'model.areAllFilesApproved, model.userRole)',
reflectToAttribute: true,
},
};
}
- static get observers() {
- return ['onInputChanged(restApi, change)'];
- }
-
static get template() {
return Polymer.html`
<style include="shared-styles">
@@ -81,6 +75,13 @@
`;
}
+ loadPropertiesAfterModelChanged() {
+ super.loadPropertiesAfterModelChanged();
+ this.modelLoader.loadUserRole();
+ this.modelLoader.loadIsCodeOwnerEnabled();
+ this.modelLoader.loadAreAllFilesApproved();
+ }
+
connectedCallback() {
super.connectedCallback();
this.expandSuggestionStateUnsubscriber = ownerState
@@ -97,26 +98,17 @@
}
}
- onInputChanged(restApi, change) {
- if ([restApi, change].includes(undefined)) return;
- this.ownerService = CodeOwnerService.getOwnerService(
- this.restApi,
- this.change
- );
-
- Promise.all([
- this.ownerService.isCodeOwnerEnabled(),
- this.ownerService.areAllFilesApproved(),
- this.ownerService.getLoggedInUserInitialRole(),
- ])
- .then(([enabled, approved, userRole]) => {
- if (enabled) {
- this.hidden = approved;
- } else {
- this.hidden = true;
- }
- this._userRole = userRole;
- });
+ _computeHidden(enabled, allFilesApproved, userRole) {
+ if (enabled === undefined ||
+ allFilesApproved === undefined ||
+ userRole === undefined) {
+ return true;
+ }
+ if (enabled) {
+ return allFilesApproved;
+ } else {
+ return true;
+ }
}
toggleControlContent() {
@@ -124,7 +116,8 @@
ownerState.expandSuggestion = this.expanded;
this.reporting.reportInteraction('toggle-suggest-owners', {
expanded: this.expanded,
- user_role: this._userRole ? this._userRole : 'UNKNOWN',
+ user_role: this.model.userRole ?
+ this.model.userRole : 'UNKNOWN',
});
}
