I'm currently integrating Privatemsg with Mollom, see #720038: Mollom integration. As usual, with private messages, everything is a bit different than with "public" content like comments/nodes (even if they are not visible to everyone). Adding protection to the form works perfectly and so does adding a "report to mollom" link. But there are two remaining issues:
- Currently, when you delete a private message, you are then redirected back to the thread unless you've deleted the last message of that thread, then you are redirected back to /messages, the messages list. However, the Cancel link should always return to the thread because there is always at least one message left. I want to do the same with the report to mollom feature. Adding a #value element for the delete destination is easy with hook_form_alter(), but I can't access it from my report delete callback so I'd have to add a custom submit callback.
- Deleting a message does by default only delete it for the current user, unless he has the 'read all private messages' permission, which allows to read and delete messages for other users. Then an additional option is added to the confirmation form which allows to delete the message for all recipients. Again, I'd like to do the same for report to mollom and as before, adding the option to the form is easy, thanks to hook_form_alter(). But this time, I really need the information in my report delete callback because I have to decide there for whom the message needs to be deleted. The only alternative is to replace the default submit callback with a custom one but I don't like that at all. *
Proposal:
So, what I'm suggesting exactly is passing $form_state['values'] as the third parameter to the report delete callback. This would be a one line patch and would not break compatibility with existing modules. I can write a patch, just wanted to ask for feedback first.
* Note: I think giving a normal user that can only delete a message for himself the report to mollom permission is a bad idea, it would be better if he would just be allowed to flag a message as possible spam and let an admin review it. But that would require either a separate feature, including a list of flagged messages or integration with flag/views (we don't have that yet, except for adding a contact link to a user/node view). Any suggestions?
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | mollom_report_delete_access-771594-30.patch | 2.52 KB | langworthy |
| #29 | mollom-HEAD.report-delete-access.29-d7.patch | 2.9 KB | sun |
| #28 | mollom-HEAD.report-delete-access.28.patch | 3.06 KB | sun |
| #15 | mollom-HEAD.delete-report-access.15.patch | 3.12 KB | sun |
| #9 | mollom-HEAD.mollom-delete-report-access.9.patch | 3.71 KB | sun |
Comments
Comment #1
sunIdeally, you should be using Webform's way of integrating with Mollom: #686136: Implement hook_mollom_form_info for submission protection by Mollom
Instead of exposing links, Mollom can integrate with the delete confirmation form of an entity. Much better UX.
Mollom's own integration code for nodes, comments, etc is rather "historical" and has been entirely removed/revamped for D7. Webform already uses the new approach for D6.
Comment #2
berdirInteresting, I thought this was only for Drupal 7. I've already converted my patch to use delete_form, just one last question..
Is there any way to limit the report to mollom feature based on a permission or something similiar? In the case of privatemsg, every user can probably delete (his own) messages, but you might not want to give them permission to report to mollom directly. As I said, in Privatemsg, everything is a bit different :)
Comment #3
berdirOk, I don't think so, so what about this.
I added a new key to the mollom_form_info hook called 'report permission' and then the report form is only displayed if that key is either not set/empty or if the current user has the defined permission. Because of the first check, this should not break anything existing.
Comment #4
berdirComment #6
berdirNo clue how this can make a blacklisting test fail. Any feedback on my idea?
#3: mollom_report_permission.patch queued for re-testing.
Comment #7
sun1) empty() should be !isset() here; i.e., you either define something valid or you don't define anything at all.
2) Interestingly, 'report access' is already used by mollom_report_access(), but is of type array there; i.e. TRUE if the current user has any of the listed permissions.
Powered by Dreditor.
Comment #8
sunLooks quite like stack overflow, but I think it makes sense.
Comment #9
sunMissed to add some magic.
Comment #10
dries commentedLet's postpone this until the next release. It is not currently a priority.
Comment #11
sunComment #12
berdir- The patch applies to 6.x-1.x-dev too and works correctly. After switching the 'report access' definition to hook_mollom_form_list() and making it an array, the report form is only shown when the user has the permission.
I think this is RTBC.
I have also updated the patch against Privatemsg to work with the patch in #9.
Comment #13
sunThis patch will need to be re-rolled.
However, I wonder whether we actually need the additional "report access callback" support, which is the only @todo in this patch and actually what makes the patch pretty complex. Does Privatemsg need it, or would "report access" (i.e., a list of user permissions) be sufficient for it?
Comment #14
berdirNo, Privatemsg only needs the permission. Patch is at #720038-23: Mollom integration.
Comment #15
sunAlright - so here's a stripped down version then.
Comment #17
berdir#15: mollom-HEAD.delete-report-access.15.patch queued for re-testing.
Comment #18
berdirI tested the patch with my updated privatemsg patch and it's working great.
I will mark this RTBC once it passes the tests (Looks to me that they are quite unstable?)
Comment #20
berdirLooks like the 7.x-1.x-dev release is broken, the patch passes all tests on 6.x-1.x-dev.
Comment #21
berdir#15: mollom-HEAD.delete-report-access.15.patch queued for re-testing.
Comment #23
berdir#15: mollom-HEAD.delete-report-access.15.patch queued for re-testing.
Comment #25
berdirJust trying to see if it passes with the 6.x-1.x-dev tests ;)
Comment #26
berdir#15: mollom-HEAD.delete-report-access.15.patch queued for re-testing.
Comment #28
sunSlightly improved.
Comment #29
sunThanks for reporting, reviewing, and testing! Committed attached patch to HEAD.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
Comment #30
langworthy commented#29 backported to 6.x-1.x
Comment #31
langworthy commentedComment #32
sunThanks! Reviewed and looks identical to the D7 patch. I think this can be safely committed.
Comment #33
dries commentedPersonally, I'd be in favor of freezing the Drupal 6 module in terms of feature requests unless really valuable. That said, I'm comfortable with this patch being committed. @sun: go for it.
Comment #34
sunThanks for reporting, reviewing, and testing! Committed to 6.x-1.x.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.