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.

Sign up for GitHub

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

Jump to bottom

Fix review conversation reply #23846

Merged
merged 1 commit into from
Apr 1, 2023
Merged

Fix review conversation reply #23846

merged 1 commit into from
Apr 1, 2023

Conversation

wxiaoguang
Copy link
Contributor

Regression of #23245

Close #23843

@wxiaoguang wxiaoguang mentioned this pull request Mar 31, 2023
Closed
@silverwind silverwind added this to the 1.20.0 milestone Mar 31, 2023
@silverwind
Copy link
Member

1.19 unaffected, right?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 31, 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 Mar 31, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 31, 2023 •
edited
Loading

1.19 unaffected, right?

Update: Yes, unaffected, no backport.

delvh
delvh reviewed Mar 31, 2023
View reviewed changes
templates/repo/diff/comment_form.tmpl
@@ -27,6 +27,7 @@
{{if $.reply}}
<button class="ui submit green tiny button btn-reply" type="submit">{{$.root.locale.Tr "repo.diff.comment.reply"}}</button>
<input type="hidden" name="reply" value="{{$.reply}}">
<input type="hidden" name="single_review" value="true">
Copy link
Member

Choose a reason for hiding this comment

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

I have a slight feeling that the current mechanism will always be somewhat erroneous:

  • With your proposed approach, it won't be possible to use replies as part of a pending review.
    The question is whether we want to allow that or not…
  • With the current approach, replies are listed as a separate review.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's the old behavior before the regression. At that time, a default form submission means comment but not pending.

Or maybe I made something wrong, but I haven't figured out. I felt that this fix works like before

Update: I think this fix is not wrong, see comment below.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • "With your proposed approach"
    That's not my proposal, that's how it worked before. Actually I think after my refactoring, the logic is slightly clearer than before, it's easier to add a correct pending behavior in the future.

  • "With the current approach, replies are listed as a separate review."
    And confirmed with Gitea 1.16, it was already like that before

So this PR only fixes the "pending" problem, I think we should leave other improvements to the future.

Screenshot of 1.16

image

Screenshot of 1.20

image

@wxiaoguang wxiaoguang marked this pull request as draft March 31, 2023 11:24
@wxiaoguang wxiaoguang marked this pull request as ready for review March 31, 2023 12:12
@wxiaoguang wxiaoguang requested a review from delvh March 31, 2023 12:14
@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 Apr 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2023 •
edited
Loading

Codecov Report

Merging #23846 (50e005c) into main (f521e88) will decrease coverage by 0.12%.
The diff coverage is 28.92%.

@@            Coverage Diff             @@
##             main   #23846      +/-   ##
==========================================
- Coverage   47.14%   47.03%   -0.12%     
==========================================
  Files        1149     1158       +9     
  Lines      151446   153179    +1733     
==========================================
+ Hits        71397    72045     +648     
- Misses      71611    72631    +1020     
- Partials     8438     8503      +65
Impacted Files Coverage Δ
cmd/dump.go 0.66% <0.00%> (-0.01%) ⬇️
cmd/mailer.go 0.00% <0.00%> (ø)
cmd/manager.go 0.00% <0.00%> (ø)
cmd/manager_logging.go 0.00% <0.00%> (ø)
cmd/migrate_storage.go 5.76% <0.00%> (-0.12%) ⬇️
cmd/restore_repo.go 0.00% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.63% <0.00%> (-0.10%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
... and 65 more

... and 72 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny lunny merged commit 9a30b2e into go-gitea:main Apr 1, 2023
@wxiaoguang wxiaoguang deleted the fix-review-reply branch April 1, 2023 06:13
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 3, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
a6741d8 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Update JS deps (go-gitea#23853)
  Added close/open button to details page of milestone (go-gitea#23877)
  Check `IsActionsToken` for LFS authentication (go-gitea#23841)
  Prefill input values in oauth settings as intended (go-gitea#23829)
  Display image size for multiarch container images (go-gitea#23821)
  Use clippie module to copy to clipboard (go-gitea#23801)
  Remove assertion debug code for show/hide refactoring (go-gitea#23576)
  [skip ci] Updated translations via Crowdin
  Remove jQuery ready usage (go-gitea#23858)
  Fix JS error when changing PR's target branch (go-gitea#23862)
  Improve action log display with control chars (go-gitea#23820)
  Fix review conversation reply (go-gitea#23846)
  Improve home page template, fix Sort dropdown menu flash (go-gitea#23856)
  Make first section on home page full width (go-gitea#23854)
  [skip ci] Updated translations via Crowdin
  Fix incorrect CORS failure detection logic (go-gitea#23844)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny approved these changes

@delvh delvh Awaiting requested review from delvh

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

Reply in the conversation of pull request UI generated a review message.
6 participants
@wxiaoguang @silverwind @codecov-commenter @lunny @delvh @GiteaBot