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

Refactor workflows endpoints for user management #2676

Merged
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
38 changes: 26 additions & 12 deletions packages/cli/src/ActiveWorkflowRunner.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable import/no-cycle */
/* eslint-disable prefer-spread */
/* eslint-disable @typescript-eslint/no-non-null-assertion */
/* eslint-disable no-param-reassign */
Expand Down Expand Up @@ -47,6 +48,9 @@ import {
WorkflowRunner,
} from '.';
import config = require('../config');
import { User } from './databases/entities/User';
import { whereClause } from './WorkflowHelpers';
import { WorkflowEntity } from './databases/entities/WorkflowEntity';

const WEBHOOK_PROD_UNREGISTERED_HINT = `The workflow must be active for a production URL to run successfully. You can activate the workflow using the toggle in the top-right of the editor. Note that unlike test URL calls, production URL calls aren't shown on the canvas (only in the executions list)`;

Expand Down Expand Up @@ -333,20 +337,30 @@ export class ActiveWorkflowRunner {
* @returns {string[]}
* @memberof ActiveWorkflowRunner
*/
async getActiveWorkflows(userId?: string): Promise<IWorkflowDb[]> {
// TODO UM: make userId mandatory?
const queryBuilder = Db.collections.Workflow!.createQueryBuilder('w');
queryBuilder.andWhere('active = :active', { active: true });
queryBuilder.select('w.id');
if (userId) {
queryBuilder.innerJoin('w.shared', 'shared');
queryBuilder.andWhere('shared.user = :userId', { userId });
async getActiveWorkflows(user?: User): Promise<IWorkflowDb[]> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, optional user since getActiveWorkflows() is used without arg by removeAll(), which is used in start.ts, ActiveWorkflowRunner.ts, and TestWebhooks.ts. Delving there would increase the scope of the PR.

let activeWorkflows: WorkflowEntity[] = [];

if (!user || user.globalRole.name === 'owner') {
activeWorkflows = await Db.collections.Workflow!.find({
select: ['id'],
where: { active: true },
});
} else {
const shared = await Db.collections.SharedWorkflow!.find({
relations: ['workflow'],
where: whereClause({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to have possibly additional filters to this whereClause allowing us to use active = true as part of it. This would reduce the need for the accumulator below without sacrificing re-usability.

I know the whereClause function already expects a third optional argument so maybe we should rethink how this can work.

Let me know if you think we should change this now or merge first and then make all changes at once when refactoring code to centralize these helpers into a model-like class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, let's do away with reduce() and keep the filters flexible, ideally merging first and then refactoring, so that we have a full view of all the call variants that we need to account for in UserManagementModel.

user,
entityType: 'workflow',
}),
});

activeWorkflows = shared.reduce<WorkflowEntity[]>((acc, cur) => {
if (cur.workflow.active) acc.push(cur.workflow);
return acc;
}, []);
}

const activeWorkflows = (await queryBuilder.getMany()) as IWorkflowDb[];
return activeWorkflows.filter(
(workflow) => this.activationErrors[workflow.id.toString()] === undefined,
);
return activeWorkflows.filter((workflow) => this.activationErrors[workflow.id] === undefined);
}

/**
Expand Down
9 changes: 7 additions & 2 deletions packages/cli/src/ResponseHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ export function sendErrorResponse(res: Response, error: ResponseError, shouldLog
res.status(httpStatusCode).json(response);
}

const isUniqueConstraintError = (error: Error) =>
['unique', 'duplicate'].some((s) => error.message.toLowerCase().includes(s));

/**
* A helper function which does not just allow to return Promises it also makes sure that
* all the responses have the same format
Expand All @@ -148,10 +151,12 @@ export function send(processFunction: (req: Request, res: Response) => Promise<a
try {
const data = await processFunction(req, res);

// Success response
sendSuccessResponse(res, data);
} catch (error) {
// Error response
if (error instanceof Error && isUniqueConstraintError(error)) {
error.message = 'There is already an entry with this name';
}

sendErrorResponse(res, error);
}
};
Expand Down
Loading