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

[GSoC2024] Added Validations, testcases to Labels Constructor #7627

Merged
merged 12 commits into from
Mar 22, 2024

Conversation

mach-12
Copy link
Contributor

@mach-12 mach-12 commented Mar 17, 2024

  1. Added new Label Validation to handle empty label name given to constructor.
  2. Fixed existing validation for Duplicate label name to make it more clear.
  3. Added tests for both.

Motivation and context

Background

  • Mentioned as anui-ux enhancement issue in Labels constructor enhancements #5729, Two problems were raised:
    1. Hint when user tries to add existing label in project is incorrect as the same code validation is triggered in both project and task.
    2. When user enters an empty label name, It is not intuitively clear that the state of the constructor is reset and skeleton is not added.

Fix

  1. Tests were written for the desired outcome.
  2. Fixed the Unique Label Validation by changing the display message to a generalized one.
Screenshot 2024-03-18 at 1 20 47 AM Screenshot 2024-03-18 at 1 21 03 AM
  1. Added a new Empty Label Validation which stops the form from submitting when label name is empty and displays an error message.
Screenshot 2024-03-18 at 1 21 20 AM

How has this been tested?

  • The issue was first triggered locally.
  • Test cases were written using Cypress, Here are the screenshots of the same.
Screenshot 2024-03-18 at 1 22 29 AM Screenshot 2024-03-18 at 1 23 51 AM

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@mach-12 mach-12 force-pushed the labels_constructor_issue_5729 branch from c65fd92 to f45d100 Compare March 17, 2024 20:34
@mach-12 mach-12 mentioned this pull request Mar 17, 2024
2 tasks
@mach-12
Copy link
Contributor Author

mach-12 commented Mar 18, 2024

hello @bsekachev @azhavoro, requesting a review.
Thanks.

Copy link
Contributor

@klakhov klakhov left a comment

Choose a reason for hiding this comment

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

  • Please, update headers with actual year in all the files you've updated
  • Add a changelog entry describing your fix

@klakhov klakhov requested review from klakhov and removed request for bsekachev and azhavoro March 18, 2024 11:34
@klakhov klakhov added the ui/ux label Mar 18, 2024
@mach-12 mach-12 requested a review from mdacoca as a code owner March 18, 2024 13:08
@mach-12
Copy link
Contributor Author

mach-12 commented Mar 18, 2024

Thanks @klakhov! Implemented your fixes:

  1. Removed custom validation, used 'required' flag.
  2. Corrected header.
  3. Fixed message in new test.
  4. Modified CHANGELOG.md

CHANGELOG.md Outdated Show resolved Hide resolved
@klakhov klakhov removed the request for review from mdacoca March 18, 2024 13:19
@mach-12 mach-12 requested a review from nmanovic as a code owner March 18, 2024 14:39
@mach-12 mach-12 requested a review from klakhov March 18, 2024 14:53
@klakhov
Copy link
Contributor

klakhov commented Mar 19, 2024

@mach-12
It looks after your changes something is wrong with tests. Could you check it?

@mach-12
Copy link
Contributor Author

mach-12 commented Mar 19, 2024

On it @klakhov

@klakhov
Copy link
Contributor

klakhov commented Mar 22, 2024

@mach-12 , it seems there is one more problem in skeletons_pipeline.js test

@mach-12
Copy link
Contributor Author

mach-12 commented Mar 22, 2024

Hello @klakhov, 38ac2cc addresses the in the skeletons_pipeline.js test. It took some time to solve because the error in pipeline was unclear. I was puzzled why adding validation caused a problem, and even more when I found out it was due to a file upload.

image

Problem Description:

  • Prior to the fix, after creating a new Skeleton Label, the addNewSkeletonLabel Cypress test would press the "Continue" button twice within the modal, attempting to close it. Which worked as two presses were equivalent to submitting and then closing the modal.
  • However, post-modification, this action caused the Constructor Modal to remain active, which in turn interfered with subsequent file uploads. This interference resulted in a failure to capture the 201 response from taskPost, as there was no required file to passed, leading to a timeout.

Issue and Fix: Pressed the 'Cancel' button.

Screenshots

  1. New Skeleton Label Submission: After the submission, the UI is expected to progress smoothly.
    New Skeleton Submitted

  2. Constructor Modal Issue: Post-submission, the modal fails to close, causing a disruption in file uploading.
    Constructor Modal Remains Open

  3. Task Post Timeout: The taskPost operation is times outs because the Submit & Open action fails to initiate due to missing file upload.
    Task Post Timeout

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Merging #7627 (af55e15) into develop (f513aa1) will decrease coverage by 0.02%.
Report is 9 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7627      +/-   ##
===========================================
- Coverage    83.47%   83.46%   -0.02%     
===========================================
  Files          373      373              
  Lines        39739    39726      -13     
  Branches      3741     3741              
===========================================
- Hits         33171    33156      -15     
- Misses        6568     6570       +2     
Components Coverage Δ
cvat-ui 79.27% <ø> (-0.02%) ⬇️
cvat-server 87.32% <100.00%> (-0.01%) ⬇️

@klakhov klakhov merged commit 0af1815 into cvat-ai:develop Mar 22, 2024
34 checks passed
@cvat-bot cvat-bot bot mentioned this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants