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

Delete a poll completly #801

Closed
Dennis1993 opened this issue Jan 29, 2020 · 14 comments · Fixed by #823
Closed

Delete a poll completly #801

Dennis1993 opened this issue Jan 29, 2020 · 14 comments · Fixed by #823
Assignees
Milestone

Comments

@Dennis1993
Copy link

It is not possible to delete a poll permanently.
If we do some polls the list will be long :D

image

@dartcafe
Copy link
Collaborator

dartcafe commented Jan 29, 2020

I waited for that issue. :-) Comes together with deleting orphaned objects #267.

@dartcafe dartcafe added this to the 1.3 milestone Jan 29, 2020
@dartcafe
Copy link
Collaborator

@v1r0x Entry issue for you?

@v1r0x
Copy link
Collaborator

v1r0x commented Jan 30, 2020

More like a re-entry 😉
There should be simply an option to permanently delete a poll in the Deleted polls section?

@dartcafe
Copy link
Collaborator

More like a re-entry
Ehm. Yes. Of course... 😁

There should be simply an option to permanently delete a poll in the Deleted polls section?
Something like this. Delete this poll permanently and/or empty trash bin (aka delete all deleted polls).

Votes, options, comments, etc. could be deleted then via a cron job, which regulary deletes all orphaned entries. See #267

@v1r0x
Copy link
Collaborator

v1r0x commented Jan 30, 2020

Votes, options, comments, etc. could be deleted then via a cron job, which regulary deletes all orphaned entries. See #267

This should be simply possible with the database using foreign keys and on delete constraints. I'll look into it and how it can be done in nc :)

@v1r0x
Copy link
Collaborator

v1r0x commented Feb 5, 2020

@dartcafe I added the logic to delete a poll completely. I'm not sure about the correct term for this action. I'm using irrevocably for now. Any suggestions?

One minor thing I couldn't fix (thanks to the horribly bad documentation of the nc apis...) is a route change after the currently selected poll gets deleted. Thus the poll is still selected. Instead it should route to polls/list/deleted or polls/list/all. Do you know the command to push a new route state?

I also added a migration to delete rows in all other polls_* tables that depend on a poll (referenced by poll_id). I couldn't figure out how migrations should be named (thanks again to the doc). Is the name ok?
Please note: This doesn't fix #267 at the moment. Only for future polls this should prevent orphaned entries. For older polls a

DELETE
FROM oc_polls_whatever
WHERE poll_id NOT IN (<IDs of currently existing polls>)

should fix that. I have to look into the doctrine documentation on how to run this raw query.

You can both @Dennis1993 and @dartcafe test this feature using the delete-poll branch

@dartcafe
Copy link
Collaborator

dartcafe commented Feb 6, 2020

@v1r0x First: Poll deletion works for me.

I'm using irrevocably for now. Any suggestions?

finally?

Do you know the command to push a new route state?

You could push the route via this.$router.push({name: 'list', params: {type: 'deleted'}}) in the vue app after successful promise return. An additionally notification there could inform the user about the final deletion.

I also added a migration to delete rows in all other polls_* tables that depend on a poll (referenced by poll_id).

Migration throws
An exception occurred while executing 'ALTER TABLE oc_polls_comments ADD CONSTRAINT FK_8843EB9B3C947C0F FOREIGN KEY (poll_id) REFERENCES oc_polls_polls (id) ON DELETE CASCADE': SQLSTATE[23000]: Integrity constraint violation: 1452 Cannot add or update a child row: a foreign key constraint fails (`nextcloud`.`#sql-51c_4ffa5c`, CONSTRAINT `FK_8843EB9B3C947C0F` FOREIGN KEY (`poll_id`) REFERENCES `oc_polls_polls` (`id`) ON DELETE CASCADE)

MySQL 5.7.28-nmm1-log

Since I ran in some trouble because of differences in the db systems, I try to avoid native db functions or capabilities and try to solve these problems inside the php code.

My idea was to simply create a tidy background job which deletes orphaned entries. In the end it could do some other tidy too.

@v1r0x
Copy link
Collaborator

v1r0x commented Feb 6, 2020

finally?

Hmm...I think I like permanently from OP :) are you ok with that?

Migration throws

From some quick googleing I think you already have orphaned entries and thus the constraint can not be added. So deleteing any orphans before the migration should fix that.
For this task a background job is overkill IMO. We also have to make sure the cron (or whatever) doesn't fail. The onDelete constraint is exactly for such tasks.
My query should be possible to convert to doctrine/nextcloud and thus work on all systems.

I'll try to fix that tomorrow

@dartcafe
Copy link
Collaborator

dartcafe commented Feb 6, 2020

Hmm...I think I like permanently from OP :) are you ok with that?

No preference here. I am OK with that.

For this task a background job is overkill IMO.

Yes, good point against that.

I think you already have orphaned entries

Without checking that, I have for sure. I'll keep them, so I can test this on my dev instance.

And BTW: Maybe you could release 1.3 from the master branch? 😁

@v1r0x
Copy link
Collaborator

v1r0x commented Feb 7, 2020

  • I replaced all instances of finally/irrevocably with permanently
  • migration is updated and should delete all orphaned entries first
  • added notifications and re-routing after successful delete

I only added the strings to the js code {{ $t(...) }} and not in one of the files in the l10n folder. Is this correct?

@dartcafe
Copy link
Collaborator

dartcafe commented Feb 7, 2020

I only added the strings to the js code {{ $t(...) }} and not in one of the files in the l10n folder. Is this correct?

Correct. As soon, as the files with new strings are merged to master, the nighly transifex bot tranferres them to transifex. Translations will arrive the next days, depending on the language support in the transifex teams.

@tessus
Copy link

tessus commented Feb 29, 2020

I'm using irrevocably for now. Any suggestions?

permanently

In English you wouldn't use irrevocably or finally in this context.

@v1r0x v1r0x closed this as completed in #823 Mar 2, 2020
Poll management automation moved this from To do to Done Mar 2, 2020
@hoyalu
Copy link

hoyalu commented Apr 3, 2024

Since this issue has been fixed a long time ago, shouldn't I find a delete delete option in each poll's three dot menu? Unfortunately, I don't. :-( All I've found is an occ command in the README.

Polls 7.0.2, Nextcloud 28.0.4

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants