Skip to content

Commit 717f2a9

Browse files
absaraswclaude
absarasw
and
claude
authored
fix(sync): freeze SKIPPED/REJECTED suggestions in syncSuggestions (SITES-44646) (#2758)
## Summary - **Root cause**: `syncSuggestions` had no guard for terminal-status suggestions in its matched-suggestions loop. When a merge function produced any difference in data (updated backlink counts, new metrics, etc.), SKIPPED and REJECTED suggestions were still added to `toUpdate` and saved via `saveMany` — bumping `updatedAt` on every scan. - **PR #2593 was a partial fix**: its `deepEqual` guard prevents spurious bumps only when data is *identical*. Any data delta still triggered a save. - **This fix**: return early for `SKIPPED` and `REJECTED` suggestions before merge functions run. Their `updatedAt` must reflect the moment the operator acted, not subsequent audit runs. - **`FIXED` is intentionally excluded**: some audits (e.g. `permissions/suggestion-data-mapper.js`) implement regression detection by transitioning `FIXED → NEW` via a custom `mergeStatusFunction`. That path must remain reachable. ## Changes - [`src/utils/data-access.js`](src/utils/data-access.js): added `FROZEN_STATUSES` early-return guard in the `matchedSuggestions.forEach` loop of `syncSuggestions` - [`test/utils/data-access.test.js`](test/utils/data-access.test.js): corrected 3 existing REJECTED tests that were asserting the buggy behavior (saveMany called on data change), added tests for SKIPPED freeze and FIXED regression path ## Test plan - [ ] `npx mocha test/utils/data-access.test.js` — 138 passing, 0 failing - [ ] SKIPPED suggestion with changed data → not saved, `updatedAt` unchanged - [ ] REJECTED suggestion with changed data → not saved, `updatedAt` unchanged - [ ] FIXED suggestion with custom mergeStatusFunction → still transitions to NEW (regression detection intact) - [ ] NEW/PENDING_VALIDATION/OUTDATED suggestions still update normally Fixes: [SITES-44646](https://jira.corp.adobe.com/browse/SITES-44646) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 94f1610 commit 717f2a9

3 files changed

Lines changed: 134 additions & 52 deletions

File tree

‎src/utils/data-access.js‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -341,14 +341,16 @@ const defaultMergeDataFunction = (existingData, newData) => ({
341341
/**
342342
* Default merge function for determining the status of an existing suggestion.
343343
* This function encapsulates the default behavior for status transitions:
344-
* - REJECTED suggestions remain REJECTED
345344
* - OUTDATED suggestions transition to PENDING_VALIDATION or NEW (possible regression)
346345
* - ERROR suggestions transition back to NEW so a re-audit re-dispatches them.
347346
* Recovers any suggestion that errored due to a transient or codefix-side bug
348347
* once the underlying fix has shipped; pages that fail every run keep
349348
* re-dispatching but the audit cadence (weekly for CWV) bounds the cost.
350349
* - Other statuses remain unchanged
351350
*
351+
* Note: SKIPPED and REJECTED suggestions never reach this function — they are
352+
* frozen upstream in syncSuggestions before mergeStatusFunction is called.
353+
*
352354
* @param {Object} existing - The existing suggestion object.
353355
* @param {Object} newDataItem - The new data item being merged.
354356
* @param {Object} context - The context object containing site and log.
@@ -358,12 +360,6 @@ export const defaultMergeStatusFunction = (existing, newDataItem, context) => {
358360
const { log, site } = context;
359361
const currentStatus = existing.getStatus();
360362

361-
if (currentStatus === SuggestionDataAccess.STATUSES.REJECTED) {
362-
// Keep REJECTED status when same suggestion appears again in audit
363-
log.debug('REJECTED suggestion found in audit. Preserving REJECTED status.');
364-
return null; // Keep existing status
365-
}
366-
367363
if (currentStatus === SuggestionDataAccess.STATUSES.OUTDATED) {
368364
log.warn('Outdated suggestion found in audit. Possible regression.');
369365
const requiresValidation = Boolean(site?.requiresValidation);
@@ -507,9 +503,25 @@ export async function syncSuggestions({
507503
return newDataKeys.has(existingKey);
508504
});
509505

506+
// SKIPPED and REJECTED suggestions are operator decisions that must not be
507+
// overwritten by subsequent scans — their updatedAt must reflect the moment
508+
// the operator acted. FIXED is intentionally excluded: some audits implement
509+
// regression detection by transitioning FIXED → NEW via a custom
510+
// mergeStatusFunction (e.g. permissions/suggestion-data-mapper.js).
511+
const FROZEN_STATUSES = [
512+
SuggestionDataAccess.STATUSES.SKIPPED,
513+
SuggestionDataAccess.STATUSES.REJECTED,
514+
];
515+
510516
matchedSuggestions.forEach((existing) => {
511517
const existingData = existing.getData();
512518
const existingKey = buildKey(existingData);
519+
520+
if (FROZEN_STATUSES.includes(existing.getStatus())) {
521+
log.debug(`Skipping ${existing.getStatus()} suggestion ${existingKey} - terminal status suggestions are never updated`);
522+
return;
523+
}
524+
513525
const newDataItem = newDataByKey.get(existingKey);
514526
// mergeDataFunction must return a new object (not mutate existingData)
515527
// for deepEqual comparison to work correctly

‎test/audits/permissions/permissions.test.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1643,8 +1643,9 @@ describe('Permissions Handler Tests', () => {
16431643

16441644
const result = mergeSuggestionStatus(mockExistingSuggestion, {}, mockContext);
16451645

1646+
// REJECTED is frozen upstream in syncSuggestions before mergeStatusFunction is called;
1647+
// if somehow reached directly, defaultMergeStatusFunction returns null (keep status).
16461648
expect(result).to.be.null;
1647-
expect(mockContext.log.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
16481649
});
16491650

16501651
it('should return null when existing suggestion is PENDING_VALIDATION', () => {
@@ -1856,8 +1857,10 @@ describe('Permissions Handler Tests', () => {
18561857

18571858
expect(result).to.deep.equal({ status: 'complete' });
18581859
expect(mockRejectedSuggestion.setStatus).to.not.have.been.called;
1859-
expect(context.dataAccess.Suggestion.saveMany).to.have.been.called;
1860-
expect(context.log.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
1860+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
1861+
expect(context.log.debug).to.have.been.calledWith(
1862+
'Skipping REJECTED suggestion admin1@/content/admin1#7d4700c5 - terminal status suggestions are never updated',
1863+
);
18611864
});
18621865

18631866
it('should create unique keys based on permission hash for same principal and path', async () => {

‎test/utils/data-access.test.js‎

Lines changed: 109 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -888,19 +888,16 @@ describe('data-access', () => {
888888
mapNewSuggestion,
889889
});
890890

891-
// Verify that REJECTED status is NOT changed (setStatus should not be called)
891+
// REJECTED suggestions are frozen before merge functions run
892892
expect(existingSuggestions[0].setStatus).to.not.have.been.called;
893-
// Verify that debug log is called with the correct message
894-
expect(mockLogger.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
895-
// With no data changes and no status change, save should be skipped
896-
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
897-
// Verify that setData is NOT called when data hasn't changed
898893
expect(existingSuggestions[0].setData).to.not.have.been.called;
899-
// Verify that skip log is called
900-
expect(mockLogger.debug).to.have.been.calledWith(sinon.match(/Skipping update for suggestion/));
894+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
895+
expect(mockLogger.debug).to.have.been.calledWith(
896+
sinon.match(/Skipping REJECTED suggestion .* terminal status/),
897+
);
901898
});
902899

903-
it('should preserve REJECTED status when data changes', async () => {
900+
it('should not save REJECTED suggestion when data changes between scans', async () => {
904901
const suggestionsData = [
905902
{ key: '1', title: 'old title', description: 'old description' },
906903
];
@@ -931,18 +928,16 @@ describe('data-access', () => {
931928
mapNewSuggestion,
932929
});
933930

934-
// Verify that REJECTED status is NOT changed (setStatus should not be called)
931+
// REJECTED suggestions must never be saved regardless of data changes
935932
expect(existingSuggestions[0].setStatus).to.not.have.been.called;
936-
// Verify that debug log is called with the correct message
937-
expect(mockLogger.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
938-
// Verify that saveMany is called with the updated suggestion
939-
expect(context.dataAccess.Suggestion.saveMany).to.have.been
940-
.calledOnceWith([existingSuggestions[0]]);
941-
// Verify that setData is called to update the data
942-
expect(existingSuggestions[0].setData).to.have.been.called;
933+
expect(existingSuggestions[0].setData).to.not.have.been.called;
934+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
935+
expect(mockLogger.debug).to.have.been.calledWith(
936+
sinon.match(/Skipping REJECTED suggestion .* terminal status/),
937+
);
943938
});
944939

945-
it('should preserve REJECTED status when data changes even if site requires validation', async () => {
940+
it('should not save REJECTED suggestion when data changes even if site requires validation', async () => {
946941
const suggestionsData = [
947942
{ key: '1', title: 'old title', url: 'https://example.com/page1' },
948943
];
@@ -958,16 +953,11 @@ describe('data-access', () => {
958953
setUpdatedBy: sinon.stub().returnsThis(),
959954
}];
960955

961-
// Data changed (url changed)
962956
const newData = [
963957
{ key: '1', title: 'old title', url: 'https://example.com/page2' },
964958
];
965959

966-
// Mock site with requiresValidation
967-
context.site = {
968-
requiresValidation: true,
969-
};
970-
960+
context.site = { requiresValidation: true };
971961
mockOpportunity.getSuggestions.resolves(existingSuggestions);
972962

973963
await syncSuggestions({
@@ -978,18 +968,15 @@ describe('data-access', () => {
978968
mapNewSuggestion,
979969
});
980970

981-
// Verify that REJECTED status is NOT changed (setStatus should not be called)
982971
expect(existingSuggestions[0].setStatus).to.not.have.been.called;
983-
// Verify that debug log is called with the correct message
984-
expect(mockLogger.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
985-
// Verify that saveMany is called with the updated suggestion
986-
expect(context.dataAccess.Suggestion.saveMany).to.have.been
987-
.calledOnceWith([existingSuggestions[0]]);
988-
// Verify that setData is called to update the data
989-
expect(existingSuggestions[0].setData).to.have.been.called;
972+
expect(existingSuggestions[0].setData).to.not.have.been.called;
973+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
974+
expect(mockLogger.debug).to.have.been.calledWith(
975+
sinon.match(/Skipping REJECTED suggestion .* terminal status/),
976+
);
990977
});
991978

992-
it('should preserve REJECTED status when nested objects and arrays change', async () => {
979+
it('should not save REJECTED suggestion when nested objects and arrays change', async () => {
993980
const suggestionsData = [
994981
{ key: '1', metrics: [{ value: 100 }], issues: [{ type: 'error1' }] },
995982
];
@@ -1005,14 +992,48 @@ describe('data-access', () => {
1005992
setUpdatedBy: sinon.stub().returnsThis(),
1006993
}];
1007994

1008-
// Nested object/array changed in data
1009995
const newData = [
1010996
{ key: '1', metrics: [{ value: 200 }], issues: [{ type: 'error2' }] },
1011997
];
1012998

1013-
context.site = {
1014-
requiresValidation: false,
1015-
};
999+
context.site = { requiresValidation: false };
1000+
mockOpportunity.getSuggestions.resolves(existingSuggestions);
1001+
1002+
await syncSuggestions({
1003+
context,
1004+
opportunity: mockOpportunity,
1005+
newData,
1006+
buildKey,
1007+
mapNewSuggestion,
1008+
});
1009+
1010+
expect(existingSuggestions[0].setStatus).to.not.have.been.called;
1011+
expect(existingSuggestions[0].setData).to.not.have.been.called;
1012+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
1013+
expect(mockLogger.debug).to.have.been.calledWith(
1014+
sinon.match(/Skipping REJECTED suggestion .* terminal status/),
1015+
);
1016+
});
1017+
1018+
it('should not save SKIPPED suggestion when data changes between scans', async () => {
1019+
const suggestionsData = [
1020+
{ key: '1', title: 'old title', url: 'https://example.com/page1' },
1021+
];
1022+
const existingSuggestions = [{
1023+
id: '1',
1024+
data: suggestionsData[0],
1025+
getData: sinon.stub().returns(suggestionsData[0]),
1026+
setData: sinon.stub(),
1027+
setRank: sinon.stub(),
1028+
save: sinon.stub().resolves(),
1029+
getStatus: sinon.stub().returns(SuggestionDataAccess.STATUSES.SKIPPED),
1030+
setStatus: sinon.stub(),
1031+
setUpdatedBy: sinon.stub().returnsThis(),
1032+
}];
1033+
1034+
const newData = [
1035+
{ key: '1', title: 'new title', url: 'https://example.com/page1' },
1036+
];
10161037

10171038
mockOpportunity.getSuggestions.resolves(existingSuggestions);
10181039

@@ -1024,12 +1045,58 @@ describe('data-access', () => {
10241045
mapNewSuggestion,
10251046
});
10261047

1027-
// Verify that REJECTED status is NOT changed (setStatus should not be called)
10281048
expect(existingSuggestions[0].setStatus).to.not.have.been.called;
1029-
expect(mockLogger.debug).to.have.been.calledWith('REJECTED suggestion found in audit. Preserving REJECTED status.');
1030-
expect(context.dataAccess.Suggestion.saveMany).to.have.been
1031-
.calledOnceWith([existingSuggestions[0]]);
1032-
expect(existingSuggestions[0].setData).to.have.been.called;
1049+
expect(existingSuggestions[0].setData).to.not.have.been.called;
1050+
expect(context.dataAccess.Suggestion.saveMany).to.not.have.been.called;
1051+
expect(mockLogger.debug).to.have.been.calledWith(
1052+
sinon.match(/Skipping SKIPPED suggestion .* terminal status/),
1053+
);
1054+
});
1055+
1056+
it('should allow FIXED suggestion to be updated when custom mergeStatusFunction detects regression', async () => {
1057+
const suggestionsData = [
1058+
{ key: '1', title: 'old title', url: 'https://example.com/page1' },
1059+
];
1060+
const existingSuggestions = [{
1061+
id: '1',
1062+
data: suggestionsData[0],
1063+
getData: sinon.stub().returns(suggestionsData[0]),
1064+
setData: sinon.stub(),
1065+
setRank: sinon.stub(),
1066+
save: sinon.stub().resolves(),
1067+
getStatus: sinon.stub().returns(SuggestionDataAccess.STATUSES.FIXED),
1068+
setStatus: sinon.stub(),
1069+
setUpdatedBy: sinon.stub().returnsThis(),
1070+
}];
1071+
1072+
const newData = [
1073+
{ key: '1', title: 'old title', url: 'https://example.com/page1' },
1074+
];
1075+
1076+
// Custom mergeStatusFunction that detects FIXED regression (e.g. permissions audit)
1077+
const regressionMergeStatus = (existing) => {
1078+
if (existing.getStatus() === SuggestionDataAccess.STATUSES.FIXED) {
1079+
return SuggestionDataAccess.STATUSES.NEW;
1080+
}
1081+
return null;
1082+
};
1083+
1084+
mockOpportunity.getSuggestions.resolves(existingSuggestions);
1085+
1086+
await syncSuggestions({
1087+
context,
1088+
opportunity: mockOpportunity,
1089+
newData,
1090+
buildKey,
1091+
mapNewSuggestion,
1092+
mergeStatusFunction: regressionMergeStatus,
1093+
});
1094+
1095+
// FIXED is NOT frozen — custom mergeStatusFunction can transition it to NEW
1096+
expect(existingSuggestions[0].setStatus).to.have.been.calledWith(
1097+
SuggestionDataAccess.STATUSES.NEW,
1098+
);
1099+
expect(context.dataAccess.Suggestion.saveMany).to.have.been.calledOnce;
10331100
});
10341101

10351102
it('should update suggestion when status changes but data does not', async () => {

0 commit comments

Comments
 (0)