Skip to content

Commit a6d1977

Browse files
committed
Only fetch permissions on first load, not on every poll
The host table polls the API every 5 seconds while a job is running. Previously every poll included include_permissions=true, causing a backend permission check on each request. Now permissions are only fetched on user-initiated calls (initial load, page change, search/filter). Poll calls omit include_permissions and instead merge the cached permissions back into each result so RowActions continues to render correctly. RowActions previously read permissions directly from the Redux store, which was overwritten by each poll response (without permissions), causing actions to disappear. It now receives permissions as a prop from the table row where the merged data lives.
1 parent 3fc0b7b commit a6d1977

3 files changed

Lines changed: 109 additions & 11 deletions

File tree

webpack/JobInvocationDetail/JobInvocationHostTable.js

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ const JobInvocationHostTable = ({
7272
const prevId = useRef(id);
7373
const pollTimeoutId = useRef(null);
7474
const currentPollParams = useRef({});
75+
const cachedPermissions = useRef({});
7576
const jobFinishedRef = useRef(jobFinished);
7677
useEffect(() => {
7778
jobFinishedRef.current = jobFinished;
@@ -159,26 +160,42 @@ const JobInvocationHostTable = ({
159160
[initialFilter, urlSearchQuery]
160161
);
161162

162-
const updateHostsState = useCallback(data => {
163-
const ids = data.data.results.map(i => i.id);
164-
setApiResponse(data.data);
163+
const updateHostsState = useCallback((data, isPoll) => {
164+
const { results } = data.data;
165+
166+
if (!isPoll) {
167+
cachedPermissions.current = Object.fromEntries(
168+
results.map(result => [result.id, result.permissions])
169+
);
170+
}
171+
172+
const mergedResults = results.map(result => ({
173+
...result,
174+
permissions: cachedPermissions.current[result.id],
175+
}));
176+
177+
const ids = mergedResults.map(i => i.id);
178+
setApiResponse({ ...data.data, results: mergedResults });
165179
setAllHostsIds(ids);
166180
setStatus(STATUS_UPPERCASE.RESOLVED);
167181
}, []);
168182

169183
// Call hosts data with params
170184
const makeApiCall = useCallback(
171-
requestParams => {
185+
(requestParams, { isPoll = false } = {}) => {
172186
dispatch(
173187
APIActions.get({
174188
key: JOB_INVOCATION_HOSTS,
175189
url: `/api/job_invocations/${id}/hosts`,
176-
params: { include_permissions: true, ...requestParams },
190+
params: {
191+
...(!isPoll && { include_permissions: true }),
192+
...requestParams,
193+
},
177194
handleSuccess: data => {
178-
updateHostsState(data);
195+
updateHostsState(data, isPoll);
179196
if (!jobFinishedRef.current) {
180197
pollTimeoutId.current = setTimeout(
181-
() => makeApiCall(currentPollParams.current),
198+
() => makeApiCall(currentPollParams.current, { isPoll: true }),
182199
5000
183200
);
184201
} else {
@@ -487,7 +504,11 @@ const JobInvocationHostTable = ({
487504
<Td key={k}>{columns[k].wrapper(result)}</Td>
488505
))}
489506
<Td isActionCell>
490-
<RowActions hostID={result.id} jobID={id} />
507+
<RowActions
508+
hostID={result.id}
509+
jobID={id}
510+
permissions={result.permissions}
511+
/>
491512
</Td>
492513
</Tr>
493514
<Tr

webpack/JobInvocationDetail/TemplateInvocationComponents/TemplateActionButtons.js

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,12 @@ const actions = ({
8181
},
8282
});
8383

84-
export const RowActions = ({ hostID, jobID }) => {
84+
export const RowActions = ({ hostID, jobID, permissions: permissionsProp }) => {
8585
const dispatch = useDispatch();
8686
const response = useSelector(selectHostFromJobInvocationHosts(hostID));
87-
if (!response?.permissions) return null;
88-
const { task, permissions } = response;
87+
const permissions = permissionsProp ?? response?.permissions;
88+
if (!permissions) return null;
89+
const { task } = response || {};
8990
const { id: taskID, cancellable: taskCancellable } = task || {};
9091
const getActions = actions({
9192
taskID,
@@ -216,4 +217,13 @@ TemplateActionButtons.defaultProps = {
216217
RowActions.propTypes = {
217218
hostID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
218219
jobID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
220+
permissions: PropTypes.shape({
221+
view_foreman_tasks: PropTypes.bool,
222+
cancel_job_invocations: PropTypes.bool,
223+
execute_jobs: PropTypes.bool,
224+
}),
225+
};
226+
227+
RowActions.defaultProps = {
228+
permissions: null,
219229
};

webpack/JobInvocationDetail/__tests__/JobInvocationHostTablePolling.test.js

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,9 @@ describe('JobInvocationHostTable polling', () => {
104104

105105
beforeEach(() => {
106106
jest.useFakeTimers({ legacyFakeTimers: true });
107+
jest
108+
.requireMock('foremanReact/components/PF4/TableIndexPage/TableIndexPage')
109+
.mockClear();
107110
apiGetSpy = jest
108111
.spyOn(api.APIActions, 'get')
109112
.mockImplementation(({ handleSuccess, ...action }) => {
@@ -219,6 +222,70 @@ describe('JobInvocationHostTable polling', () => {
219222
expect(apiGetSpy).toHaveBeenCalledTimes(1);
220223
});
221224

225+
describe('include_permissions behaviour', () => {
226+
const hostWithPermissions = {
227+
id: 1,
228+
permissions: {
229+
view_foreman_tasks: true,
230+
cancel_job_invocations: true,
231+
execute_jobs: true,
232+
},
233+
};
234+
235+
it('sends include_permissions on the first (non-poll) call', () => {
236+
const store = makeStore();
237+
renderAndTriggerFetch(store);
238+
239+
expect(apiGetSpy.mock.calls[0][0].params).toMatchObject({
240+
include_permissions: true,
241+
});
242+
});
243+
244+
it('omits include_permissions on poll calls', () => {
245+
const store = makeStore();
246+
renderAndTriggerFetch(store);
247+
248+
act(() => jest.advanceTimersByTime(5000));
249+
250+
expect(apiGetSpy.mock.calls[1][0].params).not.toHaveProperty(
251+
'include_permissions'
252+
);
253+
});
254+
255+
it('preserves permissions from the initial call in subsequent poll responses', () => {
256+
apiGetSpy.mockImplementation(({ handleSuccess, params }) => {
257+
handleSuccess &&
258+
handleSuccess({
259+
data: {
260+
results: params.include_permissions ? [hostWithPermissions] : [{ id: 1 }],
261+
total: 1,
262+
subtotal: 1,
263+
},
264+
});
265+
return { type: 'MOCK_GET' };
266+
});
267+
268+
const store = makeStore();
269+
renderAndTriggerFetch(store);
270+
271+
// Advance to the first poll — this call has no include_permissions,
272+
// so the raw result has no permissions field.
273+
act(() => jest.advanceTimersByTime(5000));
274+
275+
const MockTableIndexPage = jest.requireMock(
276+
'foremanReact/components/PF4/TableIndexPage/TableIndexPage'
277+
);
278+
const replacementResponse =
279+
MockTableIndexPage.mock.calls[
280+
MockTableIndexPage.mock.calls.length - 1
281+
][0].replacementResponse;
282+
283+
expect(replacementResponse.response.results[0].permissions).toEqual(
284+
hostWithPermissions.permissions
285+
);
286+
});
287+
});
288+
222289
it('uses the current id in the api url after id prop changes', () => {
223290
const store = makeStore();
224291
renderAndTriggerFetch(store, { id: '42' });

0 commit comments

Comments
 (0)