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

Rework job form #416

Merged
merged 4 commits into from
Jun 13, 2023
Merged

Rework job form #416

merged 4 commits into from
Jun 13, 2023

Conversation

west270
Copy link
Contributor

@west270 west270 commented Jun 1, 2023

Closes #415

Reworked job form to use react hook form. The form should be able to create a job and update a job.

Copy link
Collaborator

@obr42 obr42 left a comment

Choose a reason for hiding this comment

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

It is a bit odd to me to have the params etc then click next and do job name and trigger... I assume it helps with code reuse between command and job? Visually it might be better to have all the tabs together (optional, required, job trigger) and bucket together everything else above the tabs. Or have the trigger tabs below the regular tabs... But I'm fine being over-ruled on this given the discussion this morning at standup.

When I made a interval job and then went to edit it, not all data was pre-populated (instance, interval). Then after switching to optional tab, everything was gone when I switched back to trigger tab. Cron job was fine to edit.


I can't seem to make a job with a date trigger:

marshmallow.exceptions.ValidationError: {'trigger': {'run_date': ['Not a valid datetime.']}}

src/components/JobSettingForm/JobCronFields.tsx Outdated Show resolved Hide resolved
src/pages/JobView/UpdateJobButton.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandView.tsx Show resolved Hide resolved
src/pages/CommandView/CommandView.tsx Show resolved Hide resolved
src/pages/CommandView/CommandJobForm.tsx Show resolved Hide resolved
src/pages/CommandView/CommandJobForm.tsx Show resolved Hide resolved
src/pages/CommandView/CommandJobForm.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandFormFields.tsx Outdated Show resolved Hide resolved
src/pages/CommandView/CommandFormFields.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@obr42 obr42 left a comment

Choose a reason for hiding this comment

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

Creating a date trigger job in the past still has a not helpful error. But I'm able to make a job for a valid date so - pass?

Was able to update interval job successfully. Comment doesn't show on job detail page except in the template box, unsure if that is intentional...

LGTM, just remove debugs and make ticket for tests

src/pages/CommandView/ParameterElements/ParamTextField.tsx Outdated Show resolved Hide resolved
src/pages/JobView/UpdateJobButton.test.tsx Show resolved Hide resolved
@west270
Copy link
Contributor Author

west270 commented Jun 13, 2023

Creating a date trigger job in the past still has a not helpful error. But I'm able to make a job for a valid date so - pass?

The date trigger is working the same as old ui. It could be more of a problem on the backend since it allows a successful creation of a job it cannot schedule. I can make a ticket just to track it but since we are going for operation parity with the old ui it should be fine as is for now.

Was able to update interval job successfully. Comment doesn't show on job detail page except in the template box, unsure if that is intentional...

The comment is not for the job detail page but is for the request view page, which is why it is in the template box.

Copy link
Collaborator

@obr42 obr42 left a comment

Choose a reason for hiding this comment

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

LGTM

@obr42 obr42 merged commit 13a9ed6 into beer-garden:main Jun 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework job form
2 participants