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 attachments list in edit comment #13036

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 5, 2020

#11141 broke the appearance of dropzone attachments when editting
comments causing poorly updating lists.

This PR fixes this.

Fix #12583

Signed-off-by: Andrew Thornton [email protected]

go-gitea#11141 broke the appearance of dropzone attachments when editting
comments causing poorly updating lists.

This PR fixes this.

Fix go-gitea#12573

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added this to the 1.13.0 milestone Oct 5, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Oct 5, 2020

could be backported to 1.12

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 5, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #13036 into master will decrease coverage by 0.00%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13036      +/-   ##
==========================================
- Coverage   42.62%   42.61%   -0.01%     
==========================================
  Files         672      672              
  Lines       73785    73795      +10     
==========================================
- Hits        31448    31447       -1     
- Misses      37247    37256       +9     
- Partials     5090     5092       +2     
Impacted Files Coverage Δ
modules/upload/upload.go 78.94% <90.00%> (+2.35%) ⬆️
routers/repo/issue.go 36.86% <100.00%> (ø)
modules/indexer/stats/db.go 52.17% <0.00%> (-17.40%) ⬇️
modules/log/event.go 56.60% <0.00%> (-0.95%) ⬇️
models/repo_list.go 78.57% <0.00%> (-0.90%) ⬇️
models/error.go 34.34% <0.00%> (-0.51%) ⬇️
modules/util/timer.go 100.00% <0.00%> (+14.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e1c51c...5166d48. Read the comment docs.

@zeripath
Copy link
Contributor Author

zeripath commented Oct 6, 2020

Unfortunately I don't think this is a fix for #12573 but rather is a fix for #12583.

I'm still struggling to completely replicate #12573

modules/upload/upload.go Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
Signed-off-by: Andrew Thornton <[email protected]>
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #13036 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13036      +/-   ##
==========================================
+ Coverage   42.60%   42.62%   +0.01%     
==========================================
  Files         673      673              
  Lines       73862    73868       +6     
==========================================
+ Hits        31470    31483      +13     
  Misses      37275    37275              
+ Partials     5117     5110       -7     
Impacted Files Coverage Δ
modules/upload/upload.go 77.35% <83.33%> (+0.76%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
models/repo_mirror.go 2.38% <0.00%> (-11.91%) ⬇️
modules/cron/tasks_basic.go 87.35% <0.00%> (-3.45%) ⬇️
services/mirror/mirror.go 18.51% <0.00%> (-1.35%) ⬇️
services/pull/pull.go 40.78% <0.00%> (ø)
models/issue_comment.go 53.75% <0.00%> (+0.30%) ⬆️
models/error.go 34.85% <0.00%> (+0.50%) ⬆️
modules/log/event.go 61.79% <0.00%> (+0.94%) ⬆️
modules/queue/workerpool.go 60.00% <0.00%> (+1.22%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f27849...b1ff337. Read the comment docs.

@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 Oct 10, 2020
@6543
Copy link
Member

6543 commented Oct 10, 2020

It makes sittuation better!

but now I get this relicts after editing (no page reload):
Screenshot at 2020-10-10 23-33-20
☝️ edit add second image

after a page reload all looks fine

@zeripath
Copy link
Contributor Author

@6543 That's odd... are you sure your javascript is up to date?

@6543
Copy link
Member

6543 commented Oct 10, 2020

Yes

@zeripath
Copy link
Contributor Author

What does yes mean?

@6543
Copy link
Member

6543 commented Oct 10, 2020

I have tested the this patch on a fresh browser so there should be no old java in the cache ...

@zeripath
Copy link
Contributor Author

Check the DOM and tell me where those nodes are and what they are contained in.

…ub.com:zeripath/gitea into fix-12573-fix-attachments-list-in-edit-comment
@zeripath
Copy link
Contributor Author

I've managed to reproduce ... give me a few mins.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

zeripath commented Oct 10, 2020

Now there's one other weird thing this PR doesn't sort which is handling what happens when you remove an attachment from an empty comment... That appears not to happen unless you add some content at the same time. (which is now properly handled.)

…ntent and no attachments properly

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

OK @6543 ready again.

@6543
Copy link
Member

6543 commented Oct 10, 2020

wow - thanks!!

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

🎉

@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 Oct 10, 2020
@zeripath zeripath merged commit 7eb8460 into go-gitea:master Oct 10, 2020
@zeripath zeripath deleted the fix-12573-fix-attachments-list-in-edit-comment branch October 10, 2020 23:50
@zeripath

This comment has been minimized.

zeripath added a commit to zeripath/gitea that referenced this pull request Oct 11, 2020
Backport go-gitea#13036

go-gitea#11141 broke the appearance of dropzone attachments when editting
comments causing poorly updating lists.

This PR fixes this.

Fix go-gitea#12583

Signed-off-by: Andrew Thornton [email protected]
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 11, 2020
techknowlogick added a commit that referenced this pull request Oct 12, 2020
Backport #13036

#11141 broke the appearance of dropzone attachments when editting
comments causing poorly updating lists.

This PR fixes this.

Fix #12583

Signed-off-by: Andrew Thornton [email protected]

Co-authored-by: techknowlogick <[email protected]>
@6543 6543 mentioned this pull request Oct 12, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

While replying to an issue, changes to attachments are applied only if body is also changed
7 participants