refactor(site): Suport template version variables on template creation #6434
Conversation
BrunoQuaresma
commented
Mar 3, 2023
•
|
Related to #5980 |
| defaultValue?: string | ||
| } | ||
|
|
||
| export const VariableInput: FC<VariableInputProps> = ({ |
There was a problem hiding this comment.
Shouldn't this component be placed in components?
There was a problem hiding this comment.
This is a good question. We should try to keep code close to where it is used, if this component starts to be used in other pages like the EditTemplatePage, we should move it to the components folder.
| )} | ||
|
|
||
| {/* Variables */} | ||
| {variables && ( |
There was a problem hiding this comment.
Do you plan to cover it with unit tests?
There was a problem hiding this comment.
Yes, I plan to add tests for this.
There was a problem hiding this comment.
👍 It would be awesome to include those in this PR, unless you have a different idea.
| disabled={isSubmitting} | ||
| key={variable.name} | ||
| onChange={async (value) => { | ||
| await form.setFieldValue("user_variable_values." + index, { |
There was a problem hiding this comment.
out of curiosity: why async/await?
There was a problem hiding this comment.
This is from the Formik library and for some reason, they made this function async. I think it is strange as well.
| upload: TemplateUploadProps | ||
| starterTemplate?: TemplateExample | ||
| parameters?: ParameterSchema[] | ||
| variables?: TemplateVersionVariable[] |
There was a problem hiding this comment.
In this case, you're adjusting the CreateTemplateForm. It seems that it has a similar logic to "edit template variables", but I'm afraid that it will hard to unify both forms?
There was a problem hiding this comment.
Yes! Because this form creates a template version + the template resource so it has some mixed values. I think this is a bit "nasty" too but Idk how to make it better or clear. 🤔 open to suggestions.
There was a problem hiding this comment.
This is a valid point, those forms are different indeed 👍
|
@mtojek definitely! I just sent it first because I would like to validate the flow first. Did you have time to QA it locally? |
|
My thoughts about testing this. Unfortunately, testing all the flow like selecting a tar, uploading it, checking if there are any parameters or variable errors, showing the error and logs on the screen, etc is kinda complex. I'm wondering if we could add a storybook to check the different states like showing variables, parameters, errors, etc, and do an integration test on the form, to see if the correct data is being returned from it. I think this is simple and good enough to guarantee the flow is working as expected. If we have major issues, we can increase the coverage by doing a major integration or e2e test. Thoughts? |
|
Yes, in this case, a storybook will be a low-hanging fruit. Alternatively, you could mock relevant backend API responses, and just "assume" that the uploaded .tar file is correct. |
|
@mtojek Yes. I just don't think having them rn is valuable. IMO, the "buggy" part is the form and see if it is returning or not the correct values on submit. |
@BrunoQuaresma that sounds fine to me. If we can capture visual representations of data and errors in storybook and then use an integration test/tests to validate form submission, I think we'll be in a good spot. |