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 poll permanently #823

Merged
merged 7 commits into from
Mar 2, 2020
Merged

Delete poll permanently #823

merged 7 commits into from
Mar 2, 2020

Conversation

v1r0x
Copy link
Collaborator

@v1r0x v1r0x commented Feb 7, 2020

fix #801

Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@uni-tuebingen.de>
Signed-off-by: Vinzenz Rosenkranz <vinzenz.rosenkranz@posteo.de>
@tortuetorche
Copy link
Contributor

Hi @v1r0x,

I think, you should rebase this branch against master branch to pass tests

Have a good day,
Tortue Torche

dartcafe
dartcafe previously approved these changes Feb 7, 2020
Copy link
Collaborator

@dartcafe dartcafe left a comment

Choose a reason for hiding this comment

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

Old orphaned entries got removed within the migration and permanent deletion works too.

unprocessed log entries get deleted too. Which means, that some notificatons will never be sent under some circumstances. A valuable price IMHO.

@dartcafe
Copy link
Collaborator

dartcafe commented Feb 7, 2020

Tavis can be ignored, because it bases on a known error, which is fixed in the master branch. In the end there are no valuable tests...

@@ -55,6 +55,7 @@
['name' => 'poll#list', 'url' => '/polls/list/', 'verb' => 'GET'],
['name' => 'poll#get', 'url' => '/polls/get/{pollId}', 'verb' => 'GET'],
['name' => 'poll#delete', 'url' => '/polls/delete/{pollId}', 'verb' => 'GET'],
['name' => 'poll#deletePermanently', 'url' => '/polls/delete/permanent/{pollId}', 'verb' => 'GET'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not using the DELETE verb?

Suggested change
['name' => 'poll#deletePermanently', 'url' => '/polls/delete/permanent/{pollId}', 'verb' => 'GET'],
['name' => 'poll#deletePermanently', 'url' => '/polls/delete/permanent/{pollId}', 'verb' => 'DELETE'],

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Itried it, but it didn't work. Thus I thought this isn't supported in NC

Copy link
Contributor

@splitt3r splitt3r Feb 16, 2020

Choose a reason for hiding this comment

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

I think it should 😁 a quick research gave me:

https://github.com/nextcloud/spreed/blob/master/appinfo/routes.php#L274-L282

...
[
		'name' => 'Room#leaveRoom',
		'url' => '/api/{apiVersion}/room/{token}/participants/active',
		'verb' => 'DELETE',
		...
]
...

https://github.com/nextcloud/spreed/blob/master/src/services/participantsService.js#L75

...
const leaveConversationSync = function(token) {
	axios.delete(generateOcsUrl('apps/spreed/api/v1/room', 2) + token + '/participants/active')
}
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some strange reason it worked fine this time 😕 Maybe I did something wrong last time. Will push a fix when at my dev machine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI. Building works well on windows machines with some tools. Only signing is an issue.

@dartcafe
Copy link
Collaborator

Routes are still a little bit unsorted and could need some optimization. More verb using.

@dartcafe dartcafe added this to the 1.4 milestone Feb 18, 2020
@dartcafe dartcafe assigned dartcafe and unassigned dartcafe Feb 19, 2020
@dartcafe
Copy link
Collaborator

@v1r0x Any further to do here?

@dartcafe
Copy link
Collaborator

I solved the merge conflict and changed to ButtonDiv.
After pull, make an npm install to resolve the dependencies merged from master!

@v1r0x
Copy link
Collaborator Author

v1r0x commented Feb 29, 2020

Changing verb from get to delete is still not done. Beside that I'm ready to merge. Will fix on monday.

@dartcafe
Copy link
Collaborator

dartcafe commented Mar 1, 2020

Changing verb from get to delete is still not done

Not really important for now, IMHO. It works the way, you resolved it. As the routes need some refactoring, this could be done later, together with the rest and maybe by switching to ressources in the routes.php.

@v1r0x
Copy link
Collaborator Author

v1r0x commented Mar 2, 2020

Then lgtm :)

@dartcafe dartcafe self-requested a review March 2, 2020 18:06
Copy link
Collaborator

@dartcafe dartcafe left a comment

Choose a reason for hiding this comment

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

lgtmt
👍 🦁

@dartcafe dartcafe dismissed splitt3r’s stale review March 2, 2020 18:08

Will be solved in another PR.

@v1r0x v1r0x merged commit 5817454 into master Mar 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the delete-poll branch March 2, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete a poll completly
4 participants