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.

Sign up for GitHub

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

Jump to bottom

Additional options to control Alert Banner visibility #3764

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Additional options to control Alert Banner visibility #3764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ff0d13
feat(AlertBanner): allow banner visibility filtering by role and app …
jacob-xhio Aug 22, 2024
a84ed3e
Merge branch 'develop' into alertBannerVisibility
amcclain Aug 22, 2024
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* `GridModel` will now accept `contextMenu: false` to omit context menus.
* New bindable `AppContainerModel.intializingLoadMaskMessage` property to allow apps to customize
the loading mask message shown during app initialization.
* Added new options for controlling alert banner visibility. Replaced `clientApps` filter in the admin
alert spec to the more generic `limitTo` filter, which also allows filtering by user role and app version.

### 🐞 Bug Fixes

Expand Down
55 changes: 42 additions & 13 deletions admin/tabs/general/alertBanner/AlertBannerModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {isEqual, isMatch, sortBy, without} from 'lodash';

export class AlertBannerModel extends HoistModel {
savedValue;
roles;
@observable.ref savedPresets: PlainObject[] = [];

@managed
Expand Down Expand Up @@ -43,8 +44,8 @@ export class AlertBannerModel extends HoistModel {
initialValue: true
},
{
name: 'clientApps',
displayName: 'Client Apps',
name: 'limitTo',
displayName: 'Limit To',
initialValue: []
},
{name: 'created', readonly: true},
Expand Down Expand Up @@ -83,7 +84,7 @@ export class AlertBannerModel extends HoistModel {
formModel.values.intent,
formModel.values.iconName,
formModel.values.enableClose,
formModel.values.clientApps
formModel.values.limitTo
],
run: () => this.syncPreview(),
fireImmediately: true
Expand All @@ -92,6 +93,7 @@ export class AlertBannerModel extends HoistModel {

override async doLoadAsync(loadSpec: LoadSpec) {
await this.loadPresetsAsync();
this.roles = await this.getRolesAsync();
const {formModel} = this;
if (formModel.isDirty && loadSpec.isAutoRefresh) return;

Expand Down Expand Up @@ -120,13 +122,13 @@ export class AlertBannerModel extends HoistModel {

@action
addPreset() {
const {message, intent, iconName, enableClose, clientApps} = this.formModel.values,
const {message, intent, iconName, enableClose, limitTo} = this.formModel.values,
dateCreated = Date.now(),
createdBy = XH.getUsername();
this.savedPresets = sortBy(
[
...this.savedPresets,
{message, intent, iconName, enableClose, clientApps, dateCreated, createdBy}
{message, intent, iconName, enableClose, limitTo, dateCreated, createdBy}
],
['intent', 'message']
);
Expand All @@ -152,15 +154,16 @@ export class AlertBannerModel extends HoistModel {

@computed
get isCurrentValuesFoundInPresets() {
const {message, intent, iconName, enableClose, clientApps} = this.formModel.values;
const {message, intent, iconName, enableClose, limitTo} = this.formModel.values;
return this.savedPresets.some(
preset =>
/*
We also check equality of sets rather than just arrays for clientApps where targeted apps are the same,
but order is not guaranteed (['app', 'admin'] vs ['admin', 'app']).
We also check equality of sets rather than just arrays for limitTo where targeted apps,
app versions, and roles are the same, but order is not guaranteed
(['app', 'admin'] vs ['admin', 'app']).
*/
isMatch(preset, {message, intent, iconName, enableClose}) &&
isEqual(new Set(preset.clientApps), new Set(clientApps))
isEqual(new Set(preset.limitTo), new Set(limitTo))
);
}

Expand Down Expand Up @@ -189,7 +192,7 @@ export class AlertBannerModel extends HoistModel {
}

async saveBannerSpecAsync(spec: AlertBannerSpec) {
const {active, message, intent, iconName, enableClose, clientApps} = spec;
const {active, message, intent, iconName, enableClose, limitTo} = spec;
try {
await XH.fetchService
.postJson({
Expand All @@ -199,14 +202,29 @@ export class AlertBannerModel extends HoistModel {
.track({
category: 'Audit',
message: 'Updated Alert Banner',
data: {active, message, intent, iconName, enableClose, clientApps},
data: {active, message, intent, iconName, enableClose, limitTo},
logData: ['active']
});
} catch (e) {
XH.handleException(e);
}
}

get limitToOptions() {
return XH.clientApps
.map(app => ({
label: `App: ${app}`,
value: `app-${app}`
}))
.concat([{label: `App Version: ${XH.appVersion}`, value: `appVer-${XH.appVersion}`}])
.concat(
this.roles?.map(role => ({
label: `Role: ${role}`,
value: `role-${role}`
}))
);
Copy link
Member

Choose a reason for hiding this comment

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

Would alpha-sort these if we go this route

}

//----------------
// Implementation
//----------------
Expand All @@ -224,7 +242,7 @@ export class AlertBannerModel extends HoistModel {

private async saveInternalAsync() {
const {formModel, savedValue} = this,
{active, message, intent, iconName, enableClose, clientApps, expires, created} =
{active, message, intent, iconName, enableClose, limitTo, expires, created} =
formModel.getData();

await formModel.validateAsync();
Expand Down Expand Up @@ -290,7 +308,7 @@ export class AlertBannerModel extends HoistModel {
intent,
iconName,
enableClose,
clientApps,
limitTo,
expires: expires?.getTime(),
publishDate: preservedPublishDate ?? now,
created: created ?? now,
Expand All @@ -302,4 +320,15 @@ export class AlertBannerModel extends HoistModel {
await XH.alertBannerService.checkForBannerAsync();
await this.refreshAsync();
}

private async getRolesAsync() {
try {
const roles = await XH.fetchJson({
url: 'roleAdmin/list'
});
Copy link
Member

Choose a reason for hiding this comment

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

This service might not always be enabled - some apps don't use Hoist's built-in role management. In that case, this would throw. Looks like we should have this call roleAdmin/config first to determine if enabled. If so, go on to load the roles. If not, might end up updating the UI, depending on how much we say about roles that's visible to users.

return roles.data.map(it => it.name);
} catch (e) {
XH.handleException(e);
}
}
}
12 changes: 5 additions & 7 deletions admin/tabs/general/alertBanner/AlertBannerPanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export const alertBannerPanel = hoistCmp.factory({
});

const formPanel = hoistCmp.factory<AlertBannerModel>(({model}) => {
const {formModel} = model,
const {formModel, limitToOptions} = model,
{isDirty, isValid} = formModel;

return panel({
Expand Down Expand Up @@ -163,16 +163,14 @@ const formPanel = hoistCmp.factory<AlertBannerModel>(({model}) => {
})
}),
formField({
field: 'clientApps',
field: 'limitTo',
item: select({
enableMulti: true,
enableClear: true,
closeMenuOnSelect: false,
noOptionsMessageFn: () => 'Enter one or more app codes.',
options: XH.clientApps.map(app => ({
label: app,
value: app
}))
noOptionsMessageFn: () =>
'Enter one or more app codes, app versions, or roles.',
options: limitToOptions
}),
info: fragment(
span('Specify what apps should the banner be shown to.'),
Copy link
Member

Choose a reason for hiding this comment

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

This help text is now mismatched

Expand Down
48 changes: 42 additions & 6 deletions svc/AlertBannerService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,18 @@ export class AlertBannerService extends HoistService {
if (!this.enabled) return;

const data: AlertBannerSpec = await XH.fetchJson({url: 'xh/alertBanner'}),
{active, expires, publishDate, message, intent, iconName, enableClose, clientApps} =
data,
{active, expires, publishDate, message, intent, iconName, enableClose, limitTo} = data,
{lastDismissed, onClose} = this;

if (
!active ||
!message ||
(expires && expires < Date.now()) ||
(lastDismissed && lastDismissed > publishDate) ||
!this.isTargetedApp(clientApps)
isEmpty(limitTo) ||
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong - believe instead you want all the helper methods below to return true if no limits spec'd

!this.isTargetedApp(limitTo) ||
!this.isTargetedAppVersion(limitTo) ||
!this.isTargetedRole(limitTo)
) {
XH.hideBanner('xhAlertBanner');
} else {
Expand Down Expand Up @@ -109,8 +111,42 @@ export class AlertBannerService extends HoistService {
XH.localStorageService.set('xhAlertBanner.lastDismissed', Date.now());
};

private isTargetedApp(clientApps: string[]): boolean {
return isEmpty(clientApps) || clientApps.includes(XH.clientAppCode);
/**
* Returns `true` if there are no limitations on application or if the
* current application is included in the defined limits.
* Returns `false` otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Nit - we can leave off the "returns false otherwise" - implied. TBH these first two private methods don't really need comments - they are pretty clear on their own. Could amend the first to isTargetedClientApp for a bit of extra clarity. A comment on the role method to indicate that user needs to have at least one of the roles might be helpful.

*/
private isTargetedApp(limitTo: string[]): boolean {
return (
!limitTo.some(entry => entry.includes('app-')) ||
limitTo.includes(`app-${XH.clientAppCode}`)
);
}

/**
* Returns `true` if there are no limitations on app version or if the
* current app version is within the defined limits.
* Returns `false` otherwise.
*/
private isTargetedAppVersion(limitTo: string[]): boolean {
return (
!limitTo.some(entry => entry.includes('appVer-')) ||
limitTo.includes(`appVer-${XH.appVersion}`)
);
}

/**
* Returns `true` if there are no limitations on user role or if the
* current user has a role specified within the defined limits.
* Returns `false` otherwise.
*/
private isTargetedRole(limitTo: string[]): boolean {
if (!limitTo.some(entry => entry.includes('role-'))) return true;
for (const entry of limitTo) {
if (!entry.includes('role-')) continue;
if (XH.getUser().hasRole(entry.substring(5))) return true;
}
return false;
}
}

Expand All @@ -125,7 +161,7 @@ export interface AlertBannerSpec {
intent: Intent;
iconName: string;
enableClose: boolean;
clientApps: string[];
limitTo: string[];
created: number;
updated: number;
updatedBy: string;
Expand Down
Loading