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

Start training button #67

Merged
merged 25 commits into from
Jun 26, 2024
Merged

Start training button #67

merged 25 commits into from
Jun 26, 2024

Conversation

jewelltaylor
Copy link
Collaborator

PR Type

[Feature | Fix | Documentation | Other ]

Short Description

Clickup Ticket(s): Start Training Button

PR that adds a start button to Not Started jobs. When clicked, a call is made to the start training endpoint. During training, the job status is temporarily changed to In Progress. If the job completes successfully, the job status is changed to Finished Successfully else it is changed to Finished With Error. Here is an example of what the UI looks like with this addition:

Screenshot 2024-06-21 at 12 04 40 PM

Tests Added

Added UI tests for the start button to make sure it only appears when appropriate and behaves as expected when clicked.

await job_in_db.set_status(status, request.app.database)
return JSONResponse(content={"status": "success"})
except AssertionError as assertion_e:
return JSONResponse(content={"error": str(assertion_e)}, status_code=400)

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
florist/api/routes/server/job.py Dismissed Show dismissed Hide dismissed
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 92.53731% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.96%. Comparing base (a0c34b3) to head (be54640).

Files Patch % Lines
florist/app/jobs/page.tsx 94.64% 3 Missing ⚠️
florist/app/jobs/hooks.tsx 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
- Coverage   91.96%   91.96%   -0.01%     
==========================================
  Files          21       21              
  Lines        1270     1331      +61     
  Branches       73       50      -23     
==========================================
+ Hits         1168     1224      +56     
- Misses        102      107       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jewelltaylor jewelltaylor requested review from lotif and emersodb June 21, 2024 16:42
florist/api/db/entities.py Show resolved Hide resolved
florist/api/routes/server/training.py Show resolved Hide resolved
florist/app/jobs/page.tsx Outdated Show resolved Hide resolved
florist/app/jobs/page.tsx Outdated Show resolved Hide resolved
florist/tests/unit/app/jobs/page.test.tsx Outdated Show resolved Hide resolved
florist/app/jobs/hooks.tsx Outdated Show resolved Hide resolved
@jewelltaylor jewelltaylor requested a review from lotif June 26, 2024 14:39
florist/app/jobs/page.tsx Outdated Show resolved Hide resolved
@lotif
Copy link
Collaborator

lotif commented Jun 26, 2024

I am adding a "details" button as well, and I'm thinking if it wouldn't be cleaner to have buttons with just icons instead:

Screenshot 2024-06-26 at 11 40 07

If you like that version, it's easy to do:

            <button
                data-testid={`start-training-button-${rowId}`}
                onClick={onClick}
                className="btn btn-primary btn-sm mb-0"
                title="Start"
            >
                <i className="material-icons text-sm">play_circle_outline</i>
            </button>

@jewelltaylor jewelltaylor requested a review from lotif June 26, 2024 18:02
Copy link
Collaborator

@lotif lotif left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments!

@jewelltaylor jewelltaylor merged commit b7092dc into main Jun 26, 2024
8 checks passed
@jewelltaylor jewelltaylor deleted the start-training-button branch June 26, 2024 18:06
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.

3 participants