hide delete actions if not authorised, toggle between delete and undelete

[Miliooo]

This pull request

• Hides the delete and undelete actions from the inlcuded layout if not authorised to delete a thread by the authoriser
• Makes it possible to undelete a deleted thread

[stof]

Please revert this

[Miliooo]

thanks @stof for all his comments. Sorry about the layout changes. i'm still quite new to this, guess I really need to disable that in my IDE. I think I fixed all your remarks for now.

[merk]

Please add a newline at the end of the file

[Miliooo]

@stof @merk thanks for the review. I'm really sorry about all those little formatting issues. It's my second time I do a pull request. And i'm far from a github expert either. I'm learning lots from this. So thank you 👍

[merk]

Please squash all commits into a single commit: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

You'll need to do a force push after squashing.

[merk]

I'd also like to see this section improved, by using the Authorizer service to check if the user can actually delete threads before being given the buttons to actually delete[1], and checking the same permissions in the delete and undelete actions[2].

[1] You'll need to create a twig function called fos_message_can_delete which will proxy the call to Authorizer->canDeleteThread
[2] Checking the same Authorizer->canDeleteThread in the delete and undelete actions and throwing an AccessDeniedException from the Security component

ps- thanks for the pull request :)

[Miliooo]

@merk thanks a lot for that link, I almost got it right ;)

You are right about the fact that my implementation is very very basic. But it was already very basic to start with. You got a delete thread action on a list with deleted messages...

That being said I'm all for improving that section. I'll add an update with your proposed changes soon. Hopefully with very clean code and very nice formatting 😁

[Miliooo]

@merk About your second point
[2] Checking the same Authorizer->canDeleteThread in the delete and undelete actions and throwing an AccessDeniedException from the Security component

This is already done in
https://github.com/FriendsOfSymfony/FOSMessageBundle/blob/master/Deleter/Deleter.php#L56

It seems that the whole package is designed to keep the controllers as small as possible. But if you do that security check in the deleter.php class then there is a problem with that interface.

https://github.com/FriendsOfSymfony/FOSMessageBundle/blob/master/Deleter/DeleterInterface.php#L12

In my opinion there needs to be a @throws exception there in the phpdoc.
Then it's the responsibility for anyone who wants to implement their own deleter, to also throw that exception.

So do you want me too...

• Update the interface with a throws exception phpdoc 💡
• Do nothing... anyone who overwrites classes better know what they are doing...
• Update the controller with the check, but then we check twice (and that for a soft delete) 😕

Ow and the deleter is also in the configurable options...
https://github.com/FriendsOfSymfony/FOSMessageBundle/blob/master/Resources/doc/99-config-reference.md

[merk]

Thanks for looking into it. Dont add the check to the controller, just the template.

I am working on v3 of this bundle with some BC breaks and plan on moving the ACL to wrappers around services like the Deleter, instead of doing it inside there.

[Miliooo]

Updated the code

• We now only show the 'delete', 'undelete' action if we are authorized to do so (depending on the authorizer)
• The delete or undelete action is no longer depending on the fact if we are in a inbox, outbox or deletebox folder but the check is done based on the $thread->isDeletedByParticipant() function

Thins i don't like:

• We are talking about deleting a thread, while what we actually do is marking it as deleted.
• Deleting a thread could be something that an admin would want to do, or something that we periodically would do. Like not keeping messages older then 6 months. But in the code they always talk about 'deleting a thread' while what actually happens is marking it as deleted.

[merk]

Extra line

[merk]

Theres not much we can do about the mechanism naming at this point. I still think this is a valid 'deletion' from a users perspective. We can always introduce the concept of purging a message thread at a later date.

[Miliooo]

true we could use purge for physically deleting a thread.

I will fix your remarks later today and then I think its ready to merge

[Miliooo]

If there are no more formatting issues, or function rename issues I think this is done.

[merk]

Thanks!

[stof]

this should probably be a Twig test rather than a twig function