-
Couldn't load subscription status.
- Fork 2
Refactor status classification #418
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR centralizes frontend status handling into a single place by creating a unified status classification system, and fixes a backend error where jobs with the new status type (dict instead of string) were causing sporadic validation errors.
- Introduces
extractJobStatusfunction to handle both string and object status formats - Refactors all status classification logic to use the new centralized approach
- Adds backend status extraction function to handle dict-formatted job statuses
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| node/src/utils/function_utils.ts | Adds centralized status extraction and classification functions |
| node/src/components/data/JobSelector.tsx | Updates all status checks to use new extraction function |
| node/src/components/data/JobRow.tsx | Simplifies status handling using centralized approach |
| flaskapi/flask_workflows.py | Adds backend status extraction to handle dict-formatted statuses |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
this was done with more changes on the #419 PR, avoid merging it |
|
@alexpargon merged your changes and added those that were relevant. I think it is still useful to clearly separate the logic from the component / context that is using it - specially as status is currently checked in two places and potentially could be checked in more. |
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.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
reviewed all changes, it seems coherent to me. Feel free to comment / ask for changes, and test yourself on (local) osparc deployment (run a few more samples) before merging.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…TISFoundation/mmux_vite into refactor-status-classification
| if (jobStatus === "SUCCESS") { | ||
| return "SUCCESS"; | ||
| } | ||
| else if (jobStatus.endsWith("FAILED") || jobStatus.endsWith("FAILURE")) { |
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.
this filter does not take into account other failed states, like:
else if (["FAILED", "ABORTED"].includes(jobStatus) || (jobStatus.startsWith("JOB_") && jobStatus.endsWith("_FAILURE")))
and endsWith should not be used unless everytime you use that element in the same position
| [jobCollections, updateJobContext], | ||
| ); | ||
|
|
||
| // const autoSelectJobs = useCallback(() => { |
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.
The autoSelectJobs functions should not be removed but implemented, it was disabled as the feature which should automatically select the in-range jobs is not present, but due the fact that after every selection / deselection of a job, we refresh the moga, we should re-activate this and do it properly to save time for the user
| else if (jobStatus === "STARTED" || jobStatus === "RUNNING") { | ||
| return "RUNNING"; | ||
| } | ||
| else if (jobStatus === "PENDING" || jobStatus.startsWith("JOB_") || jobStatus.startsWith("WAITING_") || jobStatus === "PUBLISHED") { |
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.
This line catches everything that starts with JOB_ as PENDING, but JOB_ can have FAILED / FAILURE at the end or other states, so it should not only filter for the starting section with an OR (even tho the FAILURE / FAILED state is removed before in another IF statement, in the future we may add more states and they will be catched here without intention)
centralizes all frontend status handling into a single place (easier to maintain)
Fixes error in the backend when the jobs have the new status type (a dict instead of a string) - this was generating the sporadic error in SuMo validation