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: Enforce repository name length limit in dialogs #171

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

zkoppert
Copy link
Contributor

@zkoppert zkoppert commented Jun 12, 2024

Pull Request

Proposed Changes

fixes #135

This pull request limits the length of repository names in the application. The changes ensure that both the creation and editing of mirrors enforce a maximum length of 100 characters for the repository name. This is achieved through changes in the frontend and backend validation schemas.

For Reviewers

  • I want to add tests but wasn't able to find where we are already testing the mirror creation process. Is there a good spot to add a test for this? Maybe test/app.test.ts?
  • Did I miss any spots that also need the limit enforced?

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run npm run lint and fix any linting issues that have been introduced
  • run npm run test and run tests
  • If publishing new data to the public (scorecards, security scan results, code quality results, live dashboards, etc.), please request review from @jeffrey-luszcz

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, maintenance, or breaking

@github-actions github-actions bot added the fix label Jun 12, 2024
@zkoppert zkoppert marked this pull request as ready for review June 12, 2024 18:59
@zkoppert zkoppert requested a review from a team as a code owner June 12, 2024 18:59
@ajhenry
Copy link
Contributor

ajhenry commented Jun 12, 2024

Nice! You can add tests for the backend like in this example. Just pass your invalid value here

https://github.com/github-community-projects/internal-contribution-forks/blob/e5dcd387551976576228137a7357c01187ad5eb2/test/server/repos.test.ts#L245-L256

You can use this test case to base it on

@zkoppert
Copy link
Contributor Author

✅ Added a test for going over the limit cc79061
👎🏻 Not happy with how the error.message looks in the expect statement. It's a multiline string. Open to any ideas/suggestions/improvements!

Copy link
Contributor

@ajhenry ajhenry left a comment

Choose a reason for hiding this comment

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

Didn’t know about repeat neat!

@ajhenry
Copy link
Contributor

ajhenry commented Jun 12, 2024

@zkoppert I think it’s good as is but you can always do a .includes("String must contain at most 100 character(s)") on the error message

@zkoppert
Copy link
Contributor Author

✅ More readable test assertion

@zkoppert
Copy link
Contributor Author

I checked out the contributing doc and didn't see anything about deploying the change. Should I deploy before merge/after merge/not at all?

@sutterj
Copy link
Contributor

sutterj commented Jun 13, 2024

We've been deploying after merges. The deployment is from the internal repo which is (I think) why it's not mentioned here.

@zkoppert zkoppert merged commit 55bf136 into main Jun 13, 2024
12 checks passed
@zkoppert zkoppert deleted the enforce-name-limit branch June 13, 2024 13:05
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.

Backend limit on repository name length should be enforced in 'create mirror' / 'edit mirror' dialog
3 participants