diff --git a/CHANGELOG.MD b/CHANGELOG.MD index cc6c8c4f8..b69e70552 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -27,6 +27,8 @@ - A repository cloned over `ssh://` or with a user in its URL no longer gets a CLI-managed connection of its own - A self-managed GitLab installed under a path such as `/gitlab` matches its connection: the path is no longer read as part of the group - Opening a component from All components keeps the rows already picked, and closing it goes back to that list rather than to the package carrying the component +- Rule removals and edits submitted by a client that cannot know rule ids, such as the CLI reading standards from Markdown, are now matched by content instead of being dropped in silence +- A rule removal or edit that matches no rule the standard holds now reports a conflict the client can resolve, instead of being accepted and applied to nothing - A GitLab repository in a subgroup keeps its full path (`group/subgroup/repo`) when tracked from the CLI, so a token or app connection links it instead of duplicating it ## Removed diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts index 3ec8389ef..d6823439b 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts @@ -450,6 +450,146 @@ describe('StandardChangeProposalApplier', () => { ).toThrow(ChangeProposalConflictError); }); }); + + describe('when the target id names no rule', () => { + it('rewrites the rule carrying the replaced content', () => { + const rule = ruleFactory({ content: 'Old content' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].content).toBe('New content'); + }); + + it('preserves the id of the rule it matched', () => { + const rule = ruleFactory({ content: 'Old content' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].id).toBe(rule.id); + }); + + it('throws ChangeProposalConflictError when no rule carries it', () => { + const rule = ruleFactory({ content: 'Something else entirely' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + expect(() => + applier.applyChangeProposals(source, [proposal as ChangeProposal]), + ).toThrow(ChangeProposalConflictError); + }); + + describe('when several rules carry the replaced content', () => { + it('throws ChangeProposalConflictError, since none can be told from the others', () => { + const rules = [ + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Duplicated content' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Duplicated content', + newValue: 'New content', + }, + }); + + expect(() => + applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]), + ).toThrow(ChangeProposalConflictError); + }); + }); + + describe('when a reviewer adjusted the decision', () => { + it('matches the rule named by the decision, not by the payload', () => { + const rule = ruleFactory({ content: 'Reviewer chose this one' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'What the client originally sent', + newValue: 'New content', + }, + status: ChangeProposalStatus.applied, + decision: { + targetId: createRuleId('unresolved'), + oldValue: 'Reviewer chose this one', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].content).toBe('New content'); + }); + }); + }); + + describe('when the target id names a rule', () => { + it('ignores another rule sharing the replaced content', () => { + const targeted = ruleFactory({ + id: createRuleId('targeted'), + content: 'Shared content', + }); + const untouched = ruleFactory({ + id: createRuleId('untouched'), + content: 'Shared content', + }); + const source = standardVersionFactory({ + rules: [targeted, untouched], + }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: targeted.id, + oldValue: 'Shared content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[1].content).toBe( + 'Shared content', + ); + }); + }); }); describe('deleteRule', () => { @@ -512,6 +652,142 @@ describe('StandardChangeProposalApplier', () => { expect((result.version.rules ?? [])[0].id).toBe(ruleToKeep.id); }); }); + + describe('when the target id names no rule', () => { + it('removes the rule carrying the content the proposal names', () => { + const rule = ruleFactory({ content: 'To be deleted' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'To be deleted', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect(result.version.rules).toEqual([]); + }); + + it('throws ChangeProposalConflictError when no rule carries that content', () => { + const rule = ruleFactory({ content: 'Keep me' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'To be deleted', + }, + }, + }); + + expect(() => + applier.applyChangeProposals(source, [proposal as ChangeProposal]), + ).toThrow(ChangeProposalConflictError); + }); + + describe('when a reviewer adjusted the decision', () => { + it('removes the rule named by the decision, not by the payload', () => { + const rules = [ + ruleFactory({ content: 'Reviewer chose this one' }), + ruleFactory({ content: 'What the client originally sent' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'What the client originally sent', + }, + }, + status: ChangeProposalStatus.applied, + decision: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'Reviewer chose this one', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect( + (result.version.rules ?? []).map((rule) => rule.content), + ).toEqual(['What the client originally sent']); + }); + }); + + describe('when several rules carry that content', () => { + it('removes all of them, since the standard is meant to be rid of it', () => { + const rules = [ + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Keep me' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'Duplicated content', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect( + (result.version.rules ?? []).map((rule) => rule.content), + ).toEqual(['Keep me']); + }); + }); + }); + + describe('when the target id names a rule', () => { + it('keeps another rule sharing its content', () => { + const targeted = ruleFactory({ + id: createRuleId('targeted'), + content: 'Shared content', + }); + const untouched = ruleFactory({ + id: createRuleId('untouched'), + content: 'Shared content', + }); + const source = standardVersionFactory({ + rules: [targeted, untouched], + }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: targeted.id, + item: { id: targeted.id, content: 'Shared content' }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].id).toBe(untouched.id); + }); + }); }); describe('multiple changes', () => { diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts index aeb73aa92..9580ff2d3 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts @@ -2,7 +2,8 @@ import { AbstractChangeProposalApplier } from './AbstractChangeProposalApplier'; import { ChangeProposal } from '../ChangeProposal'; import { ChangeProposalType } from '../ChangeProposalType'; import { StandardVersion } from '../../standards/StandardVersion'; -import { createRuleId } from '../../standards/RuleId'; +import { createRuleId, RuleId } from '../../standards/RuleId'; +import { ChangeProposalConflictError } from './ChangeProposalConflictError'; import { isExpectedChangeProposalType } from './isExpectedChangeProposalType'; import { STANDARD_CHANGE_TYPES } from './types'; @@ -85,8 +86,24 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) ) { const rules = source.rules || []; + const targetId = changeProposal.payload.targetId; + + // A client reading standards from Markdown has no rule ids to send, so + // it names the rule by the content it is replacing instead. + let fallbackId: RuleId | undefined; + if (!rules.some((rule) => rule.id === targetId)) { + const matches = rules.filter( + (rule) => + rule.content === this.getEffectivePayload(changeProposal).oldValue, + ); + if (matches.length !== 1) { + throw new ChangeProposalConflictError(changeProposal.id); + } + fallbackId = matches[0].id; + } + const updatedRules = rules.map((rule) => { - if (rule.id !== changeProposal.payload.targetId) { + if (rule.id !== targetId && rule.id !== fallbackId) { return rule; } @@ -113,13 +130,27 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) ) { const rules = source.rules || []; - const filteredRules = rules.filter( - (rule) => rule.id !== changeProposal.payload.targetId, - ); + const targetId = changeProposal.payload.targetId; + + if (rules.some((rule) => rule.id === targetId)) { + return { + ...source, + rules: rules.filter((rule) => rule.id !== targetId), + }; + } + + // Every copy goes: the standard is meant to be rid of that content, and + // copies hold no id to tell them apart. + const removed = this.getEffectivePayload(changeProposal).item.content; + const remaining = rules.filter((rule) => rule.content !== removed); + + if (remaining.length === rules.length) { + throw new ChangeProposalConflictError(changeProposal.id); + } return { ...source, - rules: filteredRules, + rules: remaining, }; }