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

merge: release-2024-23 master -> dev #5714

Merged
merged 9 commits into from
Jun 9, 2024
5 changes: 4 additions & 1 deletion packages/admin-panel/src/autocomplete/ReduxAutocomplete.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const ReduxAutocompleteComponent = ({
placeholder,
helperText,
required,
optionValueKey,
}) => {
const [hasUpdated, setHasUpdated] = React.useState(false);
React.useEffect(() => {
Expand All @@ -51,7 +52,8 @@ const ReduxAutocompleteComponent = ({
if (hasUpdated || !initialValue) return;
const needToUpdate = allowMultipleValues
? JSON.stringify(initialValue) !== JSON.stringify(selection?.map(s => s[optionLabelKey]))
: initialValue !== selection?.[optionLabelKey];
: initialValue !== selection?.[optionValueKey];

if (needToUpdate) {
programaticallyChangeSelection(initialValue);
}
Expand Down Expand Up @@ -103,6 +105,7 @@ ReduxAutocompleteComponent.propTypes = {
selection: PropTypes.oneOfType([PropTypes.array, PropTypes.object]),
initialValue: PropTypes.oneOfType([PropTypes.array, PropTypes.object]),
required: PropTypes.bool,
optionValueKey: PropTypes.string.isRequired,
};

ReduxAutocompleteComponent.defaultProps = {
Expand Down
2 changes: 2 additions & 0 deletions packages/admin-panel/src/pages/resources/ResourcePage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ ResourcePage.propTypes = {
basePath: PropTypes.string,
hasBESAdminAccess: PropTypes.bool.isRequired,
needsBESAdminAccess: PropTypes.arrayOf(PropTypes.string),
actionLabel: PropTypes.string,
};

ResourcePage.defaultProps = {
Expand All @@ -198,4 +199,5 @@ ResourcePage.defaultProps = {
getNestedViewLink: null,
basePath: '',
needsBESAdminAccess: [],
actionLabel: 'Action',
};
2 changes: 0 additions & 2 deletions packages/admin-panel/src/routes/users/accessRequests.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ const USER_COLUMNS = [
Header: 'Edit',
type: 'bulkEdit',
source: 'user_id',
width: 150,
actionConfig: {
title: 'Edit & approve access requests',
bulkGetEndpoint: `users/{user_id}/${ACCESS_REQUESTS_ENDPOINT}`,
Expand Down Expand Up @@ -134,7 +133,6 @@ const DETAILS_COLUMNS = [
...ACCESS_REQUEST_FIELDS,
{
Header: 'Approve/decline',
width: 140,
source: 'id',
type: 'edit',
actionConfig: {
Expand Down
2 changes: 2 additions & 0 deletions packages/admin-panel/src/routes/users/permissionGroups.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const CREATE_CONFIG = {
source: 'parent_id',
editConfig: {
optionsEndpoint: 'permissionGroups',
optionLabelKey: 'name',
optionValueKey: 'id',
},
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
changePageSize,
changeResizedColumns,
changeSorting,
clearError,
confirmAction,
refreshData,
} from '../actions';
Expand Down Expand Up @@ -300,7 +301,7 @@ const DataFetchingTableComponent = memo(
}
isButtonColumn
>
Action
{actionLabel}
</HeaderDisplayCell>
)}
</TableRow>
Expand Down Expand Up @@ -470,6 +471,7 @@ const mergeProps = (stateProps, { dispatch, ...dispatchProps }, ownProps) => {
const initialiseTable = (filters = defaultFilters) => {
dispatch(changeSorting(reduxId, defaultSorting));
dispatch(changeFilters(reduxId, filters)); // will trigger a data fetch afterwards
dispatch(clearError());
};
return {
reduxId,
Expand Down
7 changes: 7 additions & 0 deletions packages/admin-panel/src/table/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
ACTION_CANCEL,
ACTION_CONFIRM,
ACTION_REQUEST,
CLEAR_ERROR,
COLUMNS_RESIZE,
DATA_CHANGE_ERROR,
DATA_CHANGE_REQUEST,
Expand Down Expand Up @@ -113,6 +114,7 @@ const refreshDataWithDebounce = debounce(
const linkHeader = parseLinkHeader(response.headers.get('Link'));
const totalRecords = parseInt(response.headers.get('X-Total-Count'), 10);
const lastPageNumber = parseInt(linkHeader.last.page, 10);

dispatch({
type: DATA_FETCH_SUCCESS,
reduxId,
Expand Down Expand Up @@ -190,6 +192,11 @@ export const deleteRecordFromTable =
reduxId,
fetchId,
errorMessage: error.message,
confirmActionMessage: '',
});
}
};

export const clearError = () => ({
type: CLEAR_ERROR,
});
1 change: 1 addition & 0 deletions packages/admin-panel/src/table/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const EXPANSIONS_CHANGE = 'EXPANSIONS_CHANGE';
export const EXPANSIONS_TAB_CHANGE = 'EXPANSIONS_TAB_CHANGE';
export const COLUMNS_RESIZE = 'COLUMNS_RESIZE';
export const SORTING_CHANGE = 'SORTING_CHANGE';
export const CLEAR_ERROR = 'CLEAR_ERROR';

export const DATA_CHANGE_ACTIONS = {
start: DATA_CHANGE_REQUEST,
Expand Down
5 changes: 5 additions & 0 deletions packages/admin-panel/src/table/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
SORTING_CHANGE,
DATA_CHANGE_REQUEST,
DEFAULT_TABLE_STATE,
CLEAR_ERROR,
} from './constants';
import { getFetchId } from './selectors';

Expand All @@ -41,6 +42,7 @@ const handleDataFetchSuccess = (payload, currentState) => {
...payload,
errorMessage: DEFAULT_TABLE_STATE.errorMessage,
fetchId: DEFAULT_TABLE_STATE.fetchId,
confirmActionMessage: DEFAULT_TABLE_STATE.confirmActionMessage,
};
};

Expand Down Expand Up @@ -87,6 +89,9 @@ const stateChanges = {
[SORTING_CHANGE]: payload => ({
...payload,
}),
[CLEAR_ERROR]: () => ({
errorMessage: DEFAULT_TABLE_STATE.errorMessage,
}),
};

export const reducer = createNestedReducer(DEFAULT_TABLE_STATE, stateChanges);
44 changes: 41 additions & 3 deletions packages/auth/src/AccessPolicyBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,58 @@
import { buildAccessPolicy } from './buildAccessPolicy';
import { buildLegacyAccessPolicy } from './buildLegacyAccessPolicy';

const getCacheKey = (userId, useLegacyFormat) => `${userId}${useLegacyFormat ? '_legacy' : ''}`;
export class AccessPolicyBuilder {
constructor(models) {
this.models = models;
this.cachedPolicyPromises = {};
this.setupCacheInvalidation();
}

resetCaches() {
this.cachedPolicyPromises = {};
}

resetCachesForUser(userId) {
this.cachedPolicyPromises[getCacheKey(userId, true)] = null; // legacy
this.cachedPolicyPromises[getCacheKey(userId, false)] = null; // modern
}

setupCacheInvalidation() {
// if a user entity permission record is changed, make sure we rebuild the associated user's
// access policy next time it is requested
this.models.userEntityPermission.addChangeHandler(
({ old_record: oldRecord, new_record: newRecord }) => {
// present in update or delete changes
if (oldRecord) {
this.resetCachesForUser(oldRecord.user_id);
}
// present in create or update changes
if (newRecord) {
this.resetCachesForUser(newRecord.user_id);
}
},
);
this.models.permissionGroup.addChangeHandler(() => this.resetCaches());
}

async getPolicyForUser(userId, useLegacyFormat = false) {
if (!userId) {
throw new Error(`Error building access policy for userId: ${userId}`);
}

if (useLegacyFormat) {
return buildLegacyAccessPolicy(this.models, userId);
const cacheKey = getCacheKey(userId, useLegacyFormat);
if (this.cachedPolicyPromises[cacheKey]) {
return this.cachedPolicyPromises[cacheKey];
}

return buildAccessPolicy(this.models, userId);
const policyPromise = useLegacyFormat
? buildLegacyAccessPolicy(this.models, userId)
: buildAccessPolicy(this.models, userId);
this.cachedPolicyPromises[cacheKey] = policyPromise;
policyPromise.catch(() => {
this.resetCachesForUser(userId); // Remove from cache if the request fails
});
return policyPromise;
}
}
70 changes: 70 additions & 0 deletions packages/auth/src/__tests__/AccessPolicyBuilder.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,74 @@ describe('AccessPolicyBuilder', () => {
},
);
});

describe('handles caching and cache invalidation', () => {
beforeEach(() => {
buildAccessPolicyMock = buildAccessPolicy.mockResolvedValue({});
buildLegacyAccessPolicyMock = buildLegacyAccessPolicy.mockResolvedValue({});
});

it('does not cache the policy if the response throws an error', async () => {
buildAccessPolicyMock = buildAccessPolicy.mockRejectedValue(
new Error('Could not build access policy'),
);

const builder = new AccessPolicyBuilder(models);
try {
await builder.getPolicyForUser(userId); // built once
} catch (error) {
// Error expected
}

try {
await builder.getPolicyForUser(userId); // built second time since it fails to build
} catch (error) {
// Error expected
}

expect(buildAccessPolicyMock).toHaveBeenCalledTimes(2);
});

it('avoids rebuilding the policy for the same user', async () => {
const builder = new AccessPolicyBuilder(models);
await builder.getPolicyForUser(userId);
await builder.getPolicyForUser(userId);
expect(buildAccessPolicyMock).toHaveBeenCalledTimes(1);
});

it('avoids rebuilding the policy for multiple users', async () => {
const builder = new AccessPolicyBuilder(models);
const userIds = ['aaa', 'bbb', 'ccc'];
// build each user's policy once
for (let i = 0; i < userIds.length; i++) {
await builder.getPolicyForUser(userIds[i]);
}
// and now fetch each of them again
for (let i = 0; i < userIds.length; i++) {
await builder.getPolicyForUser(userIds[i]);
}
// finally, a couple of extra fetches for luck
await builder.getPolicyForUser(userIds[2]);
await builder.getPolicyForUser(userIds[0]);
expect(buildAccessPolicyMock).toHaveBeenCalledTimes(3);
});

it('does rebuild if there is a change to user entity permissions', async () => {
const builder = new AccessPolicyBuilder(models);
await builder.getPolicyForUser(userId); // built once
await builder.getPolicyForUser(userId); // just fetched
notifyPermissionsChange(userId); // cache invalidated
await builder.getPolicyForUser(userId); // built a second time
expect(buildAccessPolicyMock).toHaveBeenCalledTimes(2);
});

it('does rebuild if there is a change to permission groups', async () => {
const builder = new AccessPolicyBuilder(models);
await builder.getPolicyForUser(userId); // built once
await builder.getPolicyForUser(userId); // just fetched
notifyPermissionGroupChange(); // cache invalidated
await builder.getPolicyForUser(userId); // built a second time
expect(buildAccessPolicyMock).toHaveBeenCalledTimes(2);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@ import { InputWrapper } from './InputWrapper';

const RadioGroup = styled(BaseRadioGroup)`
margin: 0;
max-width: 100%;
.MuiFormGroup-root {
width: 100%;
border: none;
flex-wrap: nowrap;
}
.MuiFormControlLabel-label {
font-size: 0.875rem;
}

.MuiFormControlLabel-root {
width: 48%;
border: 1px solid ${({ theme }) => theme.palette.divider};
Expand Down Expand Up @@ -52,7 +55,7 @@ export const EntityLevelInput = () => {
rules={{ required: 'Required' }}
render={({ ref, onChange, value }, { invalid }) => (
<RadioGroup
label="Entity level *"
label="Entity level"
name="entityLevel"
options={[
{ label: 'Country', value: 'country' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ export const getIsQuestionVisible = (
return Object.entries(dependantQuestions)[operator](([questionId, validAnswers]) => {
const answer = formData[questionId];
if (answer === undefined) return false;
return Array.isArray(validAnswers) ? validAnswers?.includes(answer) : validAnswers === answer;

// stringify the answer to compare with the validAnswers so that the types are always the same
const stringifiedAnswer = String(answer);
return Array.isArray(validAnswers)
? validAnswers?.map(validAnswer => String(validAnswer)).includes(stringifiedAnswer)
: String(validAnswers) === stringifiedAnswer;
});
};

Expand Down