-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add workflow configuration data to WorkflowDefinitions table #1026
Conversation
I am not 100% sold on the fields being integers. It would be somewhat more readable if they were strings. This would probably require changing the underlying data type of the enums to be strings, which could possibly cause a performance hit for using them as often as they are used. |
@FyreByrd Is this to get these config out of the TypeScript code and into the database? For future Workflow Definitions, we will need UI to edit/add these settings? |
Yes. I do intend to add UI for that as well, but I wasn't sure whether that should be part of this PR or a different one. I've been trying to keep DB changes as separate minimal PRs. |
@FyreByrd Yes, I think it is good to have separate PRs. You could use interactive rebasing to have the database in one commit and the UI in one comment. It is just a matter of making sure to do a Rebase and merge instead of Squash and merge. Realistically, separate PRs are probably better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
We may want to remove |
Added two fields to
WorkflowDefinitions
:ProductType
: an integer that represents theProductType
enum incommon/public/workflow.ts
AdminRequirements
: an array of integers that represent theWorkflowAdminRequirements
enum incommon/public/workflow.ts