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 bug of branch/tag selector in the issue sidebar #32744

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

hiifong
Copy link
Member

@hiifong hiifong commented Dec 6, 2024

Fix: #32731

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 6, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Dec 6, 2024
@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 Dec 6, 2024
@yp05327 yp05327 added the topic/ui Change the appearance of the Gitea UI label Dec 6, 2024
@yp05327
Copy link
Member

yp05327 commented Dec 6, 2024

Didn’t look into it, but why RepoBranchTagSelector is not used here?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 6, 2024

TBH, this selector should be completely removed.

I don't see it is useful in modern Gitea. It seems that selector was added in old days when Gitea doesn't have proper Issue/PR support.

Anyone is really using that selector?

@yp05327
Copy link
Member

yp05327 commented Dec 6, 2024

It seems that it is used only in this pkace.

@wxiaoguang
Copy link
Contributor

History: Add possibility to record branch or tag information in an issue (#780)

It does nothing more than just saving the branch/tag name into database ..............

@hiifong
Copy link
Member Author

hiifong commented Dec 6, 2024

TBH, this selector should be completely removed.

I don't see it is useful in modern Gitea. It seems that selector was added in old days when Gitea doesn't have proper Issue/PR support.

Anyone is really using that selector?

I think it is not a selector here, but a hint.

@yp05327
Copy link
Member

yp05327 commented Dec 6, 2024

I don’t know the usage of this feature if we have development section in the sidebar. They seem similar.

#31899 (comment)

@shugen002
Copy link

Just some notice:
this selector , in other word, this sidebar also used in the new issue page.
if someone want to do some refactor, don't forget to check the page.

@lunny
Copy link
Member

lunny commented Dec 6, 2024

History: Add possibility to record branch or tag information in an issue (#780)

It does nothing more than just saving the branch/tag name into database ..............

After reviewed the history, the feature aims to record which branch/tag happened of this issue. It looks like it maybe useful when reporting a bug. It's incomplete to have filter in issues page. Since Github has no such feature, we commonly record a version, OS, and other things in the issue content. Another alternative feature is label. Users can create labels with the same name as versions and track them with labels. So is this a really necessary feature? Maybe not.

@yp05327
Copy link
Member

yp05327 commented Dec 6, 2024

I think this feature can be merged into development as it is used for developers to track in which branch/tag happened of this issue.
Or at least, giving it a description and a title. For now, I think most users don’t know the correct usage which it is designed for, especially after the implementation of development section.
Or just remove it.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 7, 2024
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/frontend labels Dec 7, 2024
@hiifong
Copy link
Member Author

hiifong commented Dec 7, 2024

remove it

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 7, 2024

remove it

Although I suggested to remove it, actually "how to remove it" should be carefully designed.

Since this feature has been there for long time, we are not sure whether it is sure that nobody needs it.

If you just remove it completely and drop the column, then what if some 1.23 users come to complain: please give that selector back to me, what could you do?

Ideally the removal should be like this:

  1. only hide it from the UI in 1.23, and collect feedbacks
  2. if nobody complains and explains how to use it, remove code and drop database column in 1.24 or 1.25

@hiifong hiifong marked this pull request as draft December 7, 2024 07:10
@hiifong hiifong force-pushed the fix/disable-selector branch from 170ff1e to 410422f Compare December 7, 2024 07:40
@pull-request-size pull-request-size bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2024
@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 Dec 7, 2024
@eeyrjmr
Copy link
Contributor

eeyrjmr commented Dec 7, 2024

Two questions.

1.what is shown/managed for issues that do have this information ? Will this break something for those instances that this was previously used one
2. How is this capability still realised (options were listed at in the OP)

This might not impact how we work... Just trying to collect the information. Multiple LTS feature branches at different branching from MAIN... I guess review comments can mention what branch the chreeypicked fix should be a applied to (or rewritten for). Not sure how 31899 works ATM so I can't comment

@wxiaoguang
Copy link
Contributor

1.what is shown/managed for issues that do have this information ? Will this break something for those instances that this was previously used one

Nobody knows at the momemt, that's why it needs to collect feedbacks. This info is only stored in database and displayed on the UI, no other logic.

  1. How is this capability still realised (options were listed at in the OP)

For example: just write the branch/tag name in the issue title? then it is still "stored in database" and "displayed on the UI"

Multiple LTS feature branches at different branching from MAIN...

It could also use other approaches, eg: labels, milestones, projects to bind an issue to a "feature"

@lunny
Copy link
Member

lunny commented Dec 7, 2024

Or maybe we can migrate the data to the new table in the coming PR. Then I need to support tag

@wxiaoguang
Copy link
Contributor

Or maybe we can migrate the data to the new table in the coming PR. Then I need to support tag

Why? Have you figured out how end users use that field? Is it certain that the Ref could be used in your PR?

@eeyrjmr
Copy link
Contributor

eeyrjmr commented Dec 7, 2024

1.what is shown/managed for issues that do have this information ? Will this break something for those instances that this was previously used one

Nobody knows at the momemt, that's why it needs to collect feedbacks. This info is only stored in database and displayed on the UI, no other logic.

so if it is only for display purposes then how it is being used presently can be complimented (honestly in my dev setup its only me that does this out of 4 of us :) ). Be it labels or text, yes there are ways to indicate to the wider team what branch to consider or what tag to check the issue again. I am just nervous about what happens to the db etc if this is dropped/removed.

@wxiaoguang
Copy link
Contributor

I am just nervous about what happens to the db etc if this is dropped/removed.

So in 1.23 we only hide it, and will drop it in next release if it is not used.

@lunny
Copy link
Member

lunny commented Dec 10, 2024

Is this ready to review or merge?

@shugen002
Copy link

shugen002 commented Dec 10, 2024

Few question , will the new feature #31899 block next release if this pr merged?
In the other word, can we make sure there is something to record a tag/branch in next branch.

@lunny
Copy link
Member

lunny commented Dec 10, 2024

Few question , will the new feature #31899 block next release if this pr merged? In the other word, can we make sure there is something to record a tag/branch in next branch.

I would like #31899 merged first.

@wxiaoguang
Copy link
Contributor

In the other word, can we make sure there is something to record a tag/branch in next branch.

This PR's question is: how do you use that branch/tag selector?

@lunny
Copy link
Member

lunny commented Dec 12, 2024

I moved #31899 from v1.23. And I think that will not be a block of this PR. The purpose is different. That PR added a development sidebar section to record which branches/commits/pull requests which resolved/will resolve the issue. But this feature is recording which branch does the issue happen.

@didim99
Copy link

didim99 commented Dec 12, 2024

Anyone is really using that selector?

Yes! Our team uses this feature for more than 20 projects with many branches. When your repo has more than 2-3 branches with different stuff (features, refactoring, etc) developing on them, it is a really useful feature to specify a branch in which issue found.

@wxiaoguang
Copy link
Contributor

it is a really useful feature to specify a branch in which issue found.

@didim99 Thank you very much for the feedback, it is valuable. Just have some more questions, could you help us to understand more about "selector"?

  • If it is used to "record the branch where a problem is found", what is the difference from "writing the branch name in issue title or description"? I can see the the branch selector's value isn't even searchable .... Could "using issue title/description to record the branch" be a feasible approach?
  • Is "GitHub-like development sidebar (#31899)" good enough (or better) for your usage?

@wxiaoguang wxiaoguang added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Dec 13, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 13, 2024
@wxiaoguang wxiaoguang changed the title Prepare to remove the branch/tag selector in the issue sidebar Fix bug of branch/tag selector in the issue sidebar Dec 13, 2024
@wxiaoguang wxiaoguang marked this pull request as ready for review December 13, 2024 00:25
@wxiaoguang
Copy link
Contributor

To avoid "breaking" in 1.23, I reverted the selector with the bug fix and added more comments. Then this PR could be merged, the discussion could be continued ( #32744 (comment) ) to figure out how to improve the branch selector in the future.

@wxiaoguang wxiaoguang enabled auto-merge (squash) December 13, 2024 00:33
@wxiaoguang wxiaoguang merged commit 30008fc into go-gitea:main Dec 13, 2024
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 13, 2024
* giteaofficial/main:
  Fix bug of branch/tag selector in the issue sidebar (go-gitea#32744)
  Fix lfs migration (go-gitea#32812)
  Avoid MacOS keychain dialog in integration tests (go-gitea#32813)
  Update actionlint.yaml
  Detect whether action view branch was deleted (go-gitea#32764)
  Add "n commits" link to contributors in contributors graph page (go-gitea#32799)
  Fix "unicode escape" JS error (go-gitea#32806)
  use dedicated runners for release artifacts (go-gitea#32811)
  Make API "compare" accept commit IDs (go-gitea#32801)
  Implement update branch API (go-gitea#32433)
  Fix JS error when dropping a file to a editor without dropzone (go-gitea#32804)
  chore: use errors.New to replace fmt.Errorf with no parameters (go-gitea#32800)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/M Denotes a PR that changes 30-99 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue reference selection empty tips should not be selectable.
9 participants