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

Replace 'userxx' with 'orgxx' in all test files when the user type is org #27052

Merged
merged 8 commits into from
Sep 14, 2023

Conversation

lng2020
Copy link
Member

@lng2020 lng2020 commented Sep 13, 2023

Currently 'userxx' and 'orgxx' are both used as username in test files when the user type is org, which is confusing. This PR replaces all 'userxx' with 'orgxx' when the user type is org(user.type==1).
Some non-trivial changes

  1. Rename user3 dir to org3 in tests/git-repositories-meta
  2. Change end in issue reference because 'org3' is one char shorter than 'user3'
    ksnip_20230913-112819
  3. Change the search result number of user/repo2 because user3/repo21 can't be searched now
    ksnip_20230913-112931
  4. Change the first org name getting from API because the result is ordered by alphabet asc and now org 17 is before org25
    JW8U7NIO(J$H _YCRB36H)T
    )KFD411O4I8RB5ZOH7E0 Z3

Other modifications are just find all and replace all.
Unit tests with SQLite are all passed.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 13, 2023
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 13, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 13, 2023
lower_name: user6
name: user6
lower_name: org6
name: org6
full_name: User Six
Copy link
Member

Choose a reason for hiding this comment

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

Shoudn't we change this too for consistency.

Suggested change
full_name: User Six
full_name: Org Six

I like these changes, we need more unit test cases in Gitea. Appreciated for your initiate.
I will take a closer look today. Could you please wait for my review. If it is on priority you can go ahead and merge after other maintainers approvals.

Copy link
Member

Choose a reason for hiding this comment

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

Furture related :- Could you slice this PR into multiple Pr. easy for me to keep track of changes, Thare are 90+ files. So.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your patient review. As I mentioned in the PR information, there are only three non-trivial changes that are not 'find and replace' in IDE. I don't think this can break into different parts because it's weird that both 'user3' and 'org3' are referred to the same user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shoudn't we change this too for consistency.

I like these changes, we need more unit test cases in Gitea. Appreciated for your initiate. I will take a closer look today. Could you please wait for my review. If it is on priority you can go ahead and merge after other maintainers approvals.

Not in a rush. Take your time 👍

Copy link
Member

@puni9869 puni9869 Sep 13, 2023

Choose a reason for hiding this comment

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

I don't think this can break into different parts because it's weird that both 'user3' and 'org3' are referred to the same user.

You can use git blame to root this.

Comment on lines 365 to 372
maxReview := builder.Select("MAX(r.id)").
From("review as r").
Where(builder.In("review.type", []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest}))

subQuery := builder.Select("review.issue_id").
From("review").
Where(builder.And(
builder.In("review.type", []ReviewType{ReviewTypeRequest, ReviewTypeReject, ReviewTypeApprove}),
builder.Eq{"review.type": ReviewTypeRequest},
Copy link
Member

Choose a reason for hiding this comment

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

Heads up, when doing changes in queries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your review. Those are from my other PR and I accidentally mixed them up.

Copy link
Member

Choose a reason for hiding this comment

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

Could you point all of your PRs I can go one by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind. I already deleted this.

@puni9869 puni9869 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Sep 13, 2023
@puni9869
Copy link
Member

Does it required backport?

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 13, 2023
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 13, 2023
@CaiCandong
Copy link
Member

8f20308 (#27052) @lng2020 Invited me to trigger a github actions check for him.

@lng2020
Copy link
Member Author

lng2020 commented Sep 13, 2023

This fix (cdbfb2e) is because when getting all orgs from API, the result is ordered by alphabet asc, and the test compares the first org full name. The older name user17 is behind org25, and the newer name org 17 is before org25. So the test should change.

@techknowlogick techknowlogick enabled auto-merge (squash) September 14, 2023 02:29
@techknowlogick techknowlogick enabled auto-merge (squash) September 14, 2023 02:30
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 14, 2023
@techknowlogick techknowlogick merged commit da50be7 into go-gitea:main Sep 14, 2023
26 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 14, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 14, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 14, 2023
* giteaofficial/main:
  Display all user types and org types on admin management UI (go-gitea#27050)
  Apply lng2020 to maintainers (go-gitea#27068)
  Fix incorrect default branch label while switching between branches (go-gitea#27053)
  set version in snapcraft yaml
  Replace 'userxx' with 'orgxx' in all test files when the user type is org  (go-gitea#27052)
  [skip ci] Updated translations via Crowdin
  Load reviewer before sending notification (go-gitea#27063)
  bump all nightly builds to 16gb
  Show the repo count in code tab on both user profile and org page. (go-gitea#27048)
  Fix Fomantic's line-height causing vertical scrollbars to appear (go-gitea#26961)
  Dashboard context dropdown position fix on landing page in mobile view. (go-gitea#27047)
@techknowlogick techknowlogick modified the milestones: 1.22.0, 1.21.0 Sep 14, 2023
@lng2020 lng2020 deleted the fix/replace-name branch September 15, 2023 03:50
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants