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?

Comments

sun’s picture

Ideally, 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.

berdir’s picture

Interesting, 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 :)

berdir’s picture

Status: Active » Needs review
StatusFileSize
new914 bytes

Ok, 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.

berdir’s picture

Title: Pass all form values to report delete callback » Add a permission to control access to report form

Status: Needs review » Needs work

The last submitted patch, mollom_report_permission.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review

No clue how this can make a blacklisting test fail. Any feedback on my idea?

#3: mollom_report_permission.patch queued for re-testing.

sun’s picture

Status: Needs review » Needs work
+++ mollom.module	6 Jun 2010 17:55:06 -0000
@@ -601,7 +601,7 @@ function mollom_form_alter(&$form, &$for
-  if (isset($forms[$form_id])) {
+  if (isset($forms[$form_id]) && ($mollom_form = mollom_form_load($forms[$form_id])) && (empty($mollom_form['report access']) || user_access($mollom_form['report access']))) {

1) 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.

sun’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new3.13 KB

Looks quite like stack overflow, but I think it makes sense.

sun’s picture

Missed to add some magic.

dries’s picture

Let's postpone this until the next release. It is not currently a priority.

sun’s picture

Status: Needs review » Postponed
berdir’s picture

- 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.

sun’s picture

Status: Postponed » Needs work

This 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?

berdir’s picture

No, Privatemsg only needs the permission. Patch is at #720038-23: Mollom integration.

sun’s picture

Status: Needs work » Needs review
StatusFileSize
new3.12 KB

Alright - so here's a stripped down version then.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.delete-report-access.15.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
berdir’s picture

I 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?)

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.delete-report-access.15.patch, failed testing.

berdir’s picture

Looks like the 7.x-1.x-dev release is broken, the patch passes all tests on 6.x-1.x-dev.

berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.delete-report-access.15.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.delete-report-access.15.patch, failed testing.

berdir’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review

Just trying to see if it passes with the 6.x-1.x-dev tests ;)

berdir’s picture

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.delete-report-access.15.patch, failed testing.

sun’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new3.06 KB

Slightly improved.

sun’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)
StatusFileSize
new2.9 KB

Thanks 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.

langworthy’s picture

#29 backported to 6.x-1.x

langworthy’s picture

Status: Patch (to be ported) » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Reviewed and looks identical to the D7 patch. I think this can be safely committed.

dries’s picture

Personally, 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.

sun’s picture

Status: Reviewed & tested by the community » Fixed

Thanks 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 70adca3 on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #771594 by sun, Berdir: Added 'report access' check for report options...

  • Commit 70adca3 on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #771594 by sun, Berdir: Added 'report access' check for report options...