Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 51 additions & 4 deletions packages/approval-controller/src/ApprovalController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import {
ApprovalRequestNoResultSupportError,
EndInvalidFlowError,
MissingApprovalFlowError,
NoApprovalFlowsError,
} from './errors';

Expand Down Expand Up @@ -1015,14 +1016,15 @@ describe('approval controller', () => {
it.each([
['no options passed', undefined],
['partial options passed', {}],
['options passed', { id: 'id' }],
['options passed', { id: 'id', loadingText: 'loadingText' }],
])(
'adds flow to state and calls showApprovalRequest with %s',
(_, approvalFlowOptions?: StartFlowOptions) => {
const result = approvalController.startFlow(approvalFlowOptions);
(_, opts?: StartFlowOptions) => {
const result = approvalController.startFlow(opts);

const expectedFlow = {
id: approvalFlowOptions?.id ?? expect.any(String),
id: opts?.id ?? expect.any(String),
loadingText: opts?.loadingText ?? null,
};
expect(result).toStrictEqual(expectedFlow);
expect(showApprovalRequest).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -1061,6 +1063,51 @@ describe('approval controller', () => {
);
});
});

describe('setFlowLoadingText', () => {
const flowId = 'flowId';

beforeEach(() => {
approvalController.startFlow({ id: flowId });
});

afterEach(() => {
approvalController.endFlow({ id: flowId });
});

it('fails to set flow loading text if the flow id does not exist', () => {
expect(() =>
approvalController.setFlowLoadingText({
id: 'wrongId',
loadingText: null,
}),
).toThrow(MissingApprovalFlowError);
});

it('changes the loading text for the approval flow', () => {
const mockLoadingText = 'Mock Loading Text';
approvalController.setFlowLoadingText({
id: flowId,
loadingText: mockLoadingText,
});

expect(
approvalController.state[APPROVAL_FLOWS_STORE_KEY].find(
(flow) => flow.id === flowId,
)?.loadingText,
).toStrictEqual(mockLoadingText);
});

it('sets the loading text back to null for the approval the flow', () => {
approvalController.setFlowLoadingText({ id: flowId, loadingText: null });

expect(
approvalController.state[APPROVAL_FLOWS_STORE_KEY].find(
(flow) => flow.id === flowId,
)?.loadingText,
).toBeNull();
});
});
});

// helpers
Expand Down
50 changes: 45 additions & 5 deletions packages/approval-controller/src/ApprovalController.ts
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anyone is of the opinion that endFlow should set the loading text back to null? It might be inconvenient in the event of an approval flow within another approval flow, but I think that's going to be a very rare occurrence.

On the other hand, not clearing it after a previous approval flow could cause some interesting bugs depending in which order approval flows happen.

An alternative to this would be to add a loadingText parameter when a flow starts and set it back to default if nothing is passed.

And even though the ticket explicitly says not to, another alternative would be for the loading text to be linked to an approval flow.

Copy link
Copy Markdown
Member

@matthewwalsh0 matthewwalsh0 Jun 9, 2023

Choose a reason for hiding this comment

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

I initially wanted to do it per flow but the issue is ownership. If a nested flow exists, is the parent flow loading text displayed as it was the initial flow and has more high level context, or is the child flow loading text shown as it's more specific and more aware of what is happening?

On reflection, perhaps there is a compromise where the loading text is per flow and so setFlowLoadingText has a flowId argument, and by default the nested flow text is displayed as it's most relevant to the current work, unless the parent flow wants to be stubborn and was started with an option such as overrideNestedLoadingText?

We could be extra thorough and also have a ignoreNestedLoadingText which simply ignores all nested text even when there is no parent text.

Clearing the text on endFlow would then make sense, but not having default loading text to start with as we can't really know that until the first approval is requested.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So...

Without doing text by flowId, we can't just clear the text at endFlow, as that would inadvertently clear the parent loading text.

If we attach the flow to a specific flowId, then it won't matter. We can even come up with ways of inheriting the parent text if one is not provided.

Having said that, it's going to be difficult to predict what the functionality should exactly be if we don't have any use case for a nested approval flow.

Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
ApprovalRequestNoResultSupportError,
EndInvalidFlowError,
NoApprovalFlowsError,
MissingApprovalFlowError,
} from './errors';

const controllerName = 'ApprovalController';
Expand Down Expand Up @@ -70,6 +71,7 @@ type ShowApprovalRequest = () => void | Promise<void>;

type ApprovalFlow = {
id: string;
loadingText: string | null;
};

export type ApprovalFlowState = ApprovalFlow;
Expand Down Expand Up @@ -194,12 +196,17 @@ export type AddResult = {
resultCallbacks?: AcceptResultCallbacks;
};

export type StartFlowOptions = OptionalField<ApprovalFlow, 'id'>;
export type StartFlowOptions = OptionalField<
ApprovalFlow,
'id' | 'loadingText'
>;

export type ApprovalFlowStartResult = ApprovalFlow;

export type EndFlowOptions = Pick<ApprovalFlow, 'id'>;

export type SetFlowLoadingTextOptions = ApprovalFlow;

export type StartFlow = {
type: `${typeof controllerName}:startFlow`;
handler: ApprovalController['startFlow'];
Expand All @@ -210,6 +217,11 @@ export type EndFlow = {
handler: ApprovalController['endFlow'];
};

export type SetFlowLoadingText = {
type: `${typeof controllerName}:setFlowLoadingText`;
handler: ApprovalController['setFlowLoadingText'];
};

export type ApprovalControllerActions =
| GetApprovalsState
| ClearApprovalRequests
Expand All @@ -219,7 +231,8 @@ export type ApprovalControllerActions =
| RejectRequest
| UpdateRequestState
| StartFlow
| EndFlow;
| EndFlow
| SetFlowLoadingText;

export type ApprovalStateChange = {
type: `${typeof controllerName}:stateChange`;
Expand Down Expand Up @@ -344,6 +357,11 @@ export class ApprovalController extends BaseControllerV2<
`${controllerName}:endFlow` as const,
this.endFlow.bind(this),
);

this.messagingSystem.registerActionHandler(
`${controllerName}:setFlowLoadingText` as const,
this.setFlowLoadingText.bind(this),
);
}

/**
Expand Down Expand Up @@ -659,19 +677,20 @@ export class ApprovalController extends BaseControllerV2<
*
* @param opts - Options bag.
* @param opts.id - The id of the approval flow.
* @param opts.loadingText - The loading text that will be associated to the approval flow.
* @returns The object containing the approval flow id.
*/
startFlow(opts: StartFlowOptions = {}): ApprovalFlowStartResult {
const id = opts.id ?? nanoid();
const finalOptions = { id };
const loadingText = opts.loadingText ?? null;

this.update((draftState) => {
draftState.approvalFlows.push(finalOptions);
draftState.approvalFlows.push({ id, loadingText });
});

this._showApprovalRequest();

return { id };
return { id, loadingText };
}

/**
Expand Down Expand Up @@ -699,6 +718,27 @@ export class ApprovalController extends BaseControllerV2<
});
}

/**
* Sets the loading text for the approval flow.
*
* @param opts - Options bag.
* @param opts.id - The approval flow loading text that will be displayed.
* @param opts.loadingText - The loading text that will be associated to the approval flow.
*/
setFlowLoadingText({ id, loadingText }: SetFlowLoadingTextOptions) {
const flowIndex = this.state.approvalFlows.findIndex(
(flow) => flow.id === id,
);

if (flowIndex === -1) {
throw new MissingApprovalFlowError(id);
}

this.update((draftState) => {
draftState.approvalFlows[flowIndex].loadingText = loadingText;
});
}

/**
* Implementation of add operation.
*
Expand Down
6 changes: 6 additions & 0 deletions packages/approval-controller/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,9 @@ export class EndInvalidFlowError extends Error {
);
}
}

export class MissingApprovalFlowError extends Error {
constructor(id: string) {
super(`No approval flows found with id '${id}'.`);
}
}