Firstly, a huge thanks to everyone concerned with this fantastic module.

Scenario:
Tony has a role that is blocked from sending to a role that Sally has.
'Display link on selected content types' is set for Recipe.

Sally writes a Recipe.
Tony reads the Recipe and sees a 'Send author a message' link.

Surely that link should not be displayed?

CommentFileSizeAuthor
#28 block_role_test3.patch4.13 KBcousin_itt
#26 block_role_test2.patch2.44 KBberdir
#25 block_role_test.patch2.75 KBcousin_itt
#16 block_role_autocomplete.patch795 bytescousin_itt
#11 block_role_autocomplete.patch828 bytescousin_itt
#8 block_role_autocomplete.patch511 bytescousin_itt

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Category: support » bug
Status: Active » Postponed (maintainer needs more info)

Nope, shouldn't. But right now, we don't have any tests for this feature (role-based blocking rules) at all. I wasn't able to reproduce your error with manual testing though, so I have a few questions for you:

- Are you using the most recent version of 6.x-2.x?
- Does the link show up in the user profile (given that you have enabled the link there) of Sally for Tom?
- Is it correctly blocked when Tom tries to write Sally a message?

I did find a bug though and that is that Sally shows up as a autocomplete suggestion for Tom. That we should be able to fix and we should write a test for it.

cousin_itt’s picture

cousin_itt commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Embarrassingly, I can't reproduce this now. It was a long day when I posted, but that's no excuse.

For what it's worth, the answers to your questions:
I'm using 6.x-2.x-dev (2011-Jan-15)
Yes, links behave normally and follow role blocking rules
Yes, access is denied if I visit a crafted url to start a blocked message.

I've closed this.....apologies.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: 'Display link on the selected content types' does not respect pm_block_user settings. » Hide role-based blocked users in autocomplete and write tests for it
Status: Closed (works as designed) » Active

Ok, great.

I'm reopening and retargeting the issue to fix the bug I found in #1.

cousin_itt’s picture

cousin_itt commented

Could you do this quite nicely with hook_privatemsg_autocomplete_alter if you could get hold of the author_id?
Maybe store it in the session or change the autocomplete request to messages/autocomplete/author_id/search_string ?
Just a thought. With the author being available in $GLOBALS['user'].

cousin_itt’s picture

cousin_itt commented

Think I have a fix:

pm_block_user.module
pm_block_user_privatemsg_sql_autocomplete_alter()
after $fragments['where'][] = 'ur.rid NOT IN (' . db_placeholders($rids) . ') OR ur.rid IS NULL';
the $rids aren't added as query_args

adding the following completes the clause:

foreach($rids as $rid) {
  $fragments['query_args']['where'][] = $rid;
}

As for writing a test, that's all new to me, but am keen to learn.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That looks good. Nice catch. Should also be possible to use something like:

$fragments['query_args']['where'] = array_merge($fragments['query_args']['where'], $rids);

Would be awesome if you could create a patch for this, see http://drupal.org/patch/create.

If you are interested in learning how to write tests, I recently wrote a blogpost about it, see http://blog.worldempire.ch/de/story/writing-automated-tests-drupal-7 (Note: It's actually pretty much the same in D6).

We basically need to do something like this in a test:

- Create 2 users
- Create a role configuration that blocks one user from writing another.
- Verify that profile link is not shown anymore
- Verify that the user doesn't show up in the autocomplete
- Verify that it is not possible to manually write a message

If you want to try to create a test, I suggest you join the #drupal-games channel, then I can help you with any questions.

cousin_itt’s picture

cousin_itt commented

Great stuff.
array_merge() is the way I went in my own code in the end.
I'll get a patch together later on today but will only have time in a couple of days to look at creating tests.

cousin_itt’s picture

cousin_itt commented
StatusFileSize
new block_role_autocomplete.patch511 bytes

Here's the patch...
And cheers for the links, tutorials and encouragement.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Looks good. Setting to needs review so that the testbot picks it up.

Status: Needs review » Needs work

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

cousin_itt’s picture

cousin_itt commented
Status: Needs work » Needs review
StatusFileSize
new block_role_autocomplete.patch828 bytes

That was just a diff patch. I guess that's why it failed the test.
I've had a play with CVS, for the first time, and think I've got a patch that will pass.
I've moved it back to needs review too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, two conditions need to be TRUE so that the bot can apply a patch:

- It needs to be made from the top-level directory of the project
- The file names need to match, you can't make a xxx_new.module file and create a diff against the original file

It is possible to do that with only diff too but it gets nasty as soon as you have multiple files :) CVS certainly is easier once you have the initial setup :)

Status: Needs review » Needs work

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

cousin_itt’s picture

cousin_itt commented

Hmmm, I'm having some teething problems :) I thought I fulfilled both those conditions with the second patch?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The path needs to be without "privatemsg/", just "pm_block_user/pm_block_user.module", haven't seen that before?

From where did you create a patch and how did you check out the module?

It should work fine if you are inside the privatemsg directory.

cousin_itt’s picture

cousin_itt commented
Status: Needs work » Needs review
StatusFileSize
new block_role_autocomplete.patch795 bytes

I created a cvs_test directory then checked out the project with:
cvs -z6 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal-contrib checkout -d privatemsg contributions/modules/privatemsg
It was within the cvs_test dir that I ran cvs diff -up > the.patch

I've run it again from the privatemsg dir.

Thanks for helping me out.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Now it passed, great! :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: » 6.x-2.x-dev

Do you still plan to write some tests for this? :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Hide role-based blocked users in autocomplete and write tests for it » Write tests for role-based blocks
Category: bug » task
Status: Needs review » Active

Commited the patch, re-targeting the issue.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Write tests for role-based blocks » Write tests for role-based blocking
cousin_itt’s picture

cousin_itt commented

Yes, apologies for the delay. Lots of stuff has got in the way, not least setting up a virtual server from scratch for my first Drupal site and then a holiday from which I've only just returned (but has given me the time to reflect and decide that Drupal is the way forward).
I'm going to try and get it together this week.
You should probably prepare yourself for some daft questions :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

No problem.

Feel free to ask in here, in IRC if I'm around or you can also try http://drupal.stackexchange.com, if the question is generic enough and might be of use for others. I'm quite active there too.

You're also welcome to work on other issues, there are many which need patches or review/testing.

cousin_itt’s picture

cousin_itt commented

I'm finally getting on with this.

The main problem I have is setting up the block rule on admin/settings/messages/block
It's a dynamically generated form.
I know the form element names and the values I want to set but they only become available after the '+ Add New Rule' is pressed.
That calls an ajax request to messages/block/js which returns the rest of the form.

Are there simpleTest tools to deal with this?
Should I be looking for other ways to set these block rules such as manipulating the db directly?

Cheers,
Neil.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, they should be able to deal with that.

You can submit a form multiple times. So first, do a drupalPost() with the 'Add new Rule' button and then fill out the form values and do a drupalPost(NULL, ...). That will submit the form on the current page.

If you can't get it working (there might be an issue with that specific form), simply post your patch as far as you have and and then I'll look into it.

cousin_itt’s picture

cousin_itt commented
StatusFileSize
new block_role_test.patch2.75 KB

I tried a few things including your suggestion but to no avail.

Please find attached the patch of the work I've done so far.
Hopefully, once you've got the block role rule set up, I should be able to continue.

Cheers for looking into this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new block_role_test2.patch2.44 KB

Ok, this is working (setting to needs review to let the testbot try it too.

Two things I had to change to get it working:
- You need to use NULL on subsequent drupalPost() calls, not the first one. There you can use the URL.
- You need to use the value of the submit, not the id. t('More') instead of 'edit-pm-block-actions-more'

Some other hints for improving your code (already done in this part, remember to do so in the new parts you will add):

- Follow the coding guidelines with your comments: Have them on a separate line, upper case the first character and add a dot at the end, try to make them proper sentences
- Add docblocks for new methods and functions
- Make sure to use t() for all strings. It is being discussed if that should be done or not, but as long as core is doing it, let's stick with that. .test files aren't indexed when looking for translation candiates, so they don't fill up the database unecessary.

cousin_itt’s picture

cousin_itt commented

Outstanding, many thanks.

I was using some old simpletest documentation that said '$submit Untranslated value, id or name of the submit button.' But the actual source code does indeed stipulate using the value of the submit button. Will get some up-to-date doxygen docs for it.

cousin_itt’s picture

cousin_itt commented
StatusFileSize
new block_role_test3.patch4.13 KB

I hope this works :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Looks good to me, commited to all branches!

Status: Fixed » Closed (fixed)

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