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

Merged
merged 9 commits into from
Oct 10, 2020
Merged

Fix attachments list in edit comment #13036

merged 9 commits into from
Oct 10, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 5, 2020 •
edited
Loading

#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 art27@cantab.net

@zeripath
Fix attachments list in edit comment
5166d48 
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 <art27@cantab.net>
@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

silverwind
silverwind reviewed Oct 6, 2020
View reviewed changes
modules/upload/upload.go Outdated Show resolved Hide resolved
silverwind
silverwind reviewed Oct 6, 2020
View reviewed changes
web_src/js/index.js Outdated Show resolved Hide resolved
@zeripath
as per @silverwind
4763d4b 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Fix the issue with dz.emit in go-gitea#12573
efa5a43 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Oct 6, 2020
Closed
7 tasks
@codecov-io
Copy link

codecov-io commented Oct 10, 2020 •
edited
Loading

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.

@zeripath
Merge branch 'fix-12573-fix-attachments-list-in-edit-comment' of gith…
d532474 
…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.

@zeripath
fix issue noted by @6543
b1b74e8 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

zeripath commented Oct 10, 2020 •
edited
Loading

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.)

@zeripath
Handle changing empty comment with an attachment to a comment with co…
b1ff337 
…ntent and no attachments properly

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

OK @6543 ready again.

@6543
Copy link
Member

6543 commented Oct 10, 2020

wow - thanks!!

6543
6543 approved these changes Oct 10, 2020
View reviewed changes
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.

Sign in to view
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 11, 2020
@zeripath
Fix attachments list in edit comment (go-gitea#13036)
80babbd 
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 art27@cantab.net
@zeripath zeripath mentioned this pull request Oct 11, 2020
Merged
@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
@zeripath @techknowlogick
Fix attachments list in edit comment (#13036) (#13097)
d4af0df 
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 art27@cantab.net

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@6543 6543 mentioned this pull request Oct 12, 2020
Closed
@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.
Reviewers

@silverwind silverwind silverwind left review comments

@6543 6543 6543 approved these changes

@lafriks lafriks lafriks approved these changes

Assignees
No one assigned
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
Milestone
1.13.0
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
@zeripath @codecov-commenter @codecov-io @6543 @silverwind @lafriks @GiteaBot