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

Fix secret key import error #1281

Merged
merged 1 commit into from
May 22, 2024
Merged

Fix secret key import error #1281

merged 1 commit into from
May 22, 2024

Conversation

OKendigelyan
Copy link
Contributor

@OKendigelyan OKendigelyan commented May 22, 2024

Proposed changes

Task link

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

Screenshots

Add the screenshots of how the app used to look like and how it looks now

Before Now
image image

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

Copy link

github-actions bot commented May 22, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 89.64% 3598/4014
🟢 Branches 81.97% 1259/1536
🟢 Functions 87.92% 1099/1250
🟢 Lines 89.57% 3402/3798

Test suite run success

1397 tests passing in 181 suites.

Report generated by 🧪jest coverage report action from 9baa572

Copy link
Contributor

@serjonya-trili serjonya-trili left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@asiia-trilitech asiia-trilitech left a comment

Choose a reason for hiding this comment

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

Thanks Oleg - the code looks great!

</form>
{isEncrypted && (
<FormControl marginTop="20px" isInvalid={!!errors.password}>
<PasswordInput data-testid="password" inputName="password" minLength={0} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to check the reasoning for minLength={0}

I think we have minLength=8 in Umami 🤔
However, for random secret keys it makes sense to me to allow different values

Copy link
Contributor

Choose a reason for hiding this comment

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

@OKendigelyan could you please add a test for the error being displayed correctly?

And another one for having passwords with length less than 8?

Copy link
Contributor Author

@OKendigelyan OKendigelyan May 22, 2024

Choose a reason for hiding this comment

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

@asiia-trilitech if I understood you correctly, there is already a test for correct error display - 'shows an error when the password is invalid'.
This implementation assumes any password, so the test for this case already exists - 'shows an error if the password is invalid'.

@OKendigelyan OKendigelyan force-pushed the fix-secret-key-import-error branch from dbf9166 to 9baa572 Compare May 22, 2024 11:32
@OKendigelyan OKendigelyan merged commit 6cfa249 into main May 22, 2024
4 checks passed
@OKendigelyan OKendigelyan deleted the fix-secret-key-import-error branch May 22, 2024 11:40
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