nextcloud / polls

🗳️ Polls app for Nextcloud
https://apps.nextcloud.com/apps/polls
GNU Affero General Public License v3.0
255 stars 73 forks source link

Delete a poll completly #801

Closed Dennis1993 closed 4 years ago

Dennis1993 commented 4 years ago

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

image

dartcafe commented 4 years ago

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

dartcafe commented 4 years ago

@v1r0x Entry issue for you?

v1r0x commented 4 years ago

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

dartcafe commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I'm using irrevocably for now. Any suggestions?

permanently

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

hoyalu commented 6 months ago

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

github-actions[bot] commented 4 months ago

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.