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

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Jun 13, 2024

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
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
Contributor Author

GVodyanov commented Jun 18, 2024

@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
@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

@GretaD
Copy link
Contributor

GretaD commented Jul 15, 2024

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/comment-deleting-with-activities branch from 094b38e to c4422cd Compare July 15, 2024 10:44
@ChristophWurst
Copy link
Member

conflicts -> needs rebase + rebuild

@GVodyanov GVodyanov force-pushed the fix/comment-deleting-with-activities branch 2 times, most recently from a52fcf2 to 6123505 Compare July 15, 2024 12:37
@ChristophWurst
Copy link
Member

/compile amend /

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory V <scratchx@gmx.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command nextcloud-command force-pushed the fix/comment-deleting-with-activities branch from 6123505 to bd7c29c Compare July 15, 2024 13:38
@ChristophWurst ChristophWurst merged commit 8c571dd into master Jul 15, 2024
111 checks passed
@ChristophWurst ChristophWurst deleted the fix/comment-deleting-with-activities branch July 15, 2024 14:20
Copy link

welcome bot commented Jul 15, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

Copy link

backportbot bot commented Jul 15, 2024

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/45848/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick bd7c29c3

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/45848/stable29

Error: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

Copy link

backportbot bot commented Jul 15, 2024

The backport to stable28 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable28
git pull origin stable28

# Create the new backport branch
git checkout -b backport/45848/stable28

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick bd7c29c3

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/45848/stable28

Error: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@susnux
Copy link
Contributor

susnux commented Jul 19, 2024

The backport to stable28 failed. Please do this backport manually.

When doing the backports @GVodyanov please directly include #46643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants