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/comment deleting with activities installed #45848

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix/comment deleting with activities installed #45848

wants to merge 2 commits into from
+115 −18

Conversation

GVodyanov
Copy link

@GVodyanov GVodyanov commented Jun 13, 2024 •
edited
Loading

Summary

Fixes issue where you:

  • Delete a comment
  • Add a new comment before the old comment gets deleted in the back-end
  • See deleted comment again

TODO

  • Add store for synchronizing deleted comments between the mixin and activities

Checklist

@GVodyanov GVodyanov added the 2. developing Work in progress label Jun 13, 2024
@GVodyanov GVodyanov self-assigned this Jun 13, 2024
@GVodyanov GVodyanov changed the title fix(comments): comment deleting with activities installed Fix/comment deleting with activities installed Jun 13, 2024
@GVodyanov GVodyanov marked this pull request as ready for review June 17, 2024 10:35
@GVodyanov GVodyanov added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 17, 2024
@susnux susnux requested a review from ShGKme June 17, 2024 12:05
ShGKme
ShGKme reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I don't think it makes sense to introduce a global storage in the app just to store ids or deleted comments in 2-3 components.

If storing deleted state in the comment it self is a problem, it's enough to store it in the parent.

Besides, it seems that the initial issue is that adding a comment triggers an entire tab reload through loading state. What's when Comment instances are being destroyed and re-mounted without old deleted state.

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
@GVodyanov
Copy link
Author

GVodyanov commented Jun 18, 2024 •
edited
Loading

@ShGKme Would you have any suggestions on how to not make the tab reload then? I'm having trouble figuring out the activities API, thank you :)

@ShGKme
Copy link
Contributor

ShGKme commented Jun 26, 2024

So, I made a deeper look at the app.

With activity app case it is indeed not possible to store data in the parent, because on the Activity tab there is no parent between comments and the form. Every comment is a new Vue app. So, as a simple workaround from the server side, it is a good workaround.

A better solution would be to adjust Activity API to not reload everything on change. But with the current approach, it is not so simple.

So, let's fix Ferdinand's suggestion and go with it for now

cc @GretaD

@susnux
Copy link
Contributor

susnux commented Jun 26, 2024

A better solution would be to adjust Activity API to not reload everything on change. But with the current approach, it is not so simple.

💯 agree

@GretaD GretaD requested review from susnux and ShGKme July 1, 2024 11:29
@GretaD GretaD force-pushed the fix/comment-deleting-with-activities branch from 794c0bd to 68ba038 Compare July 1, 2024 13:04
@GretaD
Copy link
Contributor

GretaD commented Jul 2, 2024

/backport to stable28

@GretaD
Copy link
Contributor

GretaD commented Jul 2, 2024

/backport to stable29

@GretaD
Copy link
Contributor

GretaD commented Jul 8, 2024

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/comment-deleting-with-activities branch from 68ba038 to f60b4f5 Compare July 8, 2024 09:14
@GretaD
Copy link
Contributor

GretaD commented Jul 8, 2024

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/comment-deleting-with-activities branch from f60b4f5 to 243d763 Compare July 8, 2024 09:47
@ChristophWurst
Copy link
Member

The branch is outdated. Other PRs touched the frontend bundles. It will need a rebase and a recompile.

@GretaD GretaD force-pushed the fix/comment-deleting-with-activities branch from 243d763 to 52ab6a0 Compare July 8, 2024 09:58
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 8, 2024
@GretaD
Copy link
Contributor

GretaD commented Jul 8, 2024

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/comment-deleting-with-activities branch from 52ab6a0 to 79aac6e Compare July 8, 2024 10:31
@susnux susnux added the bug label Jul 9, 2024
@GVodyanov @susnux
fix(comments): comment deleting with activities installed
0c50a58 
Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>
@GVodyanov @susnux
chore(comments): register Pinia and clean up
01cce6e 
Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@susnux susnux force-pushed the fix/comment-deleting-with-activities branch from 79aac6e to 01cce6e Compare July 9, 2024 17:37
@susnux
Copy link
Contributor

susnux commented Jul 9, 2024

@GVodyanov

/home/runner/actions-runner/_work/server/server/apps/comments/src/store/deletedCommentLimbo.js
Error: 6:1 error Expected space or tab after '//' in comment spaced-comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@susnux susnux susnux approved these changes

@GretaD GretaD Awaiting requested review from GretaD

@kesselb kesselb Awaiting requested review from kesselb

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees

@GretaD GretaD

@GVodyanov GVodyanov

Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request bug
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@GVodyanov @ShGKme @susnux @GretaD @ChristophWurst @solracsf