Add tests for reply only permission

Your patch has wrong format. Have a look at this link as it describes proper patch format: http://drupal.org/patch/create

The patch will also need more work - it will also need to evaluate if a thread_id has been set along with the reply function and then return TRUE or FALSE.

The -dev version has been updated, there is now a new permission after write privatemsg so you need to update your patch.

I'm not sure if "reply only" is a good name for a permission as it is quite generic. Not sure what to use instead.

Also, your check in validate_message() is too simple, it will also allow to send new messages through the api functions. So you need to check tha either a user is allowed to write private messages OR (he can reply and there is a $message['thread_id']).

PS: No need to change privatemsg_reply(), as all api functions call validate_message() and it can be handled in there.

     'delete privatemsg',
+	'reply only'
   );

The indentation is a bit off and it should have an ending ",", Drupal Coding Standard...

-  if (privatemsg_user_access('write privatemsg')) {
+  if (privatemsg_user_access('rabarber') || privatemsg_user_access('reply only')) {

I assume that is a left-over of your debugging :)

Regarding your issue, first, make sure that the issue does not happen without your patch. also make sure to check the value of $form['privatemsg']['#access']. If you can't figure it out, clean up the above things and I will give it a try.

See #579148: Bugs in new/reply form, it is a bug in the current -dev version

     'delete privatemsg',
+	  'reply only',
   );

There is a tab instead of spaces, so it looks wrong in the patch (and tabs aren't allowed anyway for exactly that reason).

-  if (privatemsg_user_access('write privatemsg')) {
+  if (privatemsg_user_access('reply only')) {

Now you removed too much. This would block users with "write privatemsg" permission. You need to check for write privatemsg OR reply only.

-  $form['privatemsg'] = array(
-    '#type'               => 'fieldset',
-    '#access'             => privatemsg_user_access('write privatemsg'),
-  );
+  if (privatemsg_user_access('write privatemsg')) {
+      $form['privatemsg'] = array(
+        '#type'               => 'fieldset',
+        '#access'             => privatemsg_user_access('write privatemsg'),
+      );
+  }
+  else {
+      $form['privatemsg'] = array(
+        '#type'               => 'fieldset',
+        '#access'             => privatemsg_user_access('reply only'),
+      );
+  }

Hm. "'#access' => privatemsg_user_access('write privatemsg') || privatemsg_user_access('reply only')" should work too here. However, you probably have to check if you are in reply mode ($thread_id is set) or not. So you need to update your if to check for empty($thread_id) and if true, only check for write else check for both with the above statement.

About the name of the permission. What about "reply to thread"?

What about "reply to private messages" since that contains more or less the module name in it. I'm wondering if it is clear that write privatemsg allows to reply too. D7 will allow us to solve that by adding a description. maybe even go for "only reply to private messages"?

Also, please name your next patch whatever.diff or .patch, that makes it easier to review with http://drupal.org/project/dreditor

I'm interested in this functionality. Has there been any updates recently? If not, I can try and pitch in where needed...

Yeah, a re-roll is needed here, see my comment above.

Some other things that just came to my mind:

- maybe we can make privatemsg_user_access() a bit more intelligent, so that checking for reply permission also checks for the write permission
- A simple test would be great (create user a with write permission, user b with reply, write message from user a to b, check that user b can reply but not write new messages)

Well, do you recommend we work off one of the existing patches on here, or sorta start from scratch? If we should work off a patch, which do you feel is the closest to the correct functionality?

You can use the patch in #10 I think, atleast what's applying of that. A big chunk of it will not, since the privatemsg_view and privatemsg_new functions have been moved to privatemsg.pages.inc.

subscribe

I Used the code from patch at #10 and placed it in the latest 6.x-2.x-dev release (20-4-2010).
I called the permissions : "reply only privatemsg"

See attached patches

Thanks for working on this...

- You should provide them as a single patch. This is easier if you use CVS but it requires more initial work/learning. Do you know http://drupal.org/patch?
- It seems that you diffed them in the wrong order, it removes the changes instead of adding them..
- Not really necessary but some tests would be awesome! (See also #13) I can help you with that, ping me in #drupal when I'm around or ask here if you have any questions.

The new patch file as requested

I don't know anything about testing. So I'll try to contact you @ #drupal .... If you have some references about making the tests, please let me know.

New correct patch based on CVS

+++ privatemsg.module	21 Apr 2010 08:51:45 -0000
@@ -1283,7 +1284,7 @@ function privatemsg_reply($thread_id, $b
   $messages = array('error' => array(), 'warning' => array());
-  if (!privatemsg_user_access('write privatemsg', $message['author'])) {
+  if (!privatemsg_user_access('write privatemsg', $message['author']) && (!privatemsg_user_access('reply only privatemsg', $message['author']) && isset($message['thread_id']))) {
     // no need to do further checks in this case...

I'm not sure if that statement is correct, need to test it.

Apart from that, the patch looks good now, looking forward to your tests!

Hint: To actually test the above, you probably need create two separate test cases. One which uses the UI (that will probably never even trigger the above code) and a second with uses the api functions privatemsg_new_thread() and privatemsg_reply().

Powered by Dreditor.

Ok, the if is wrong then. You probably need something like this:

if (!(privatemsg_user_access('write privatemsg', $message['author']) || (privatemsg_user_access('reply only privatemsg', $message['author']) && isset($message['thread_id'])))) {

}

Another try...

#21: privatemsg-572374.patch queued for re-testing.

Just wondering if you still plan to work on some tests?

Don't want to get this fall behind again... :)

First I have to satisfy my customer with a new website. After the site is live (2-3 weeks) I will look into the test.

No problem, no need to hurry.

One bump.
I am going to test the current patch. (in #25)
This permission would be neat.

Testing finished (quickly but it's sure) :
Well, it works. I had 3 users. 1 has the reply only, 1 has the reply and create, 1 has create
Here is what functions well :
- You have no link to create a new message if you only have the reply only permission
- Visiting messages/new is restricted, of course
- You have a reply form in messages you've received, however.

- The 2 users with the create permission can create new threads.

I think this can make it into the 2.x-dev branch ?

Allowing users of a role to have "Reply Only" is a great new feature I think, but I'm confused on it being a permission. So if you want a role to be allowed to Send and Reply, then we don't check this on the permissions page? It sounds more of a setting (ie that would be found in the User Blocking Rules page) where it would say something like "Send and Reply Role access" and roles can be added to the "Reply Only". Just wondering from a usability perspective..sorry I don't know programming =( Thanks!

Hi,

If you want a role to be able to Send or Reply, you must check the Write new messages permission.
If you do so, adding the Reply only permission is redundant.

I think it's more natural as a permission.

#31 is a valid concern, but I think a permission is more natural too. Giving the permission to users which have create private message permission is redundant but does not break anything, permissions can only ever allow something, not disallow.

We can probably address this better in Drupal 7 because we have descriptions for permissions there. We just need a good description there then, help is very welcome on that part.

Adding "reply only" to the user blocking rules would be a interesting feature, but imho a different one, which should be handled in a different issue if someone is interested in that.

Thanks for testing, I will probably commit this soon, still hoping for a few simple tests as described in #13/#19. Anyone is invited to try writing a few tests, you can ask me directly in #drupal or here, I'm happy to guide you through the process of writing a test.

#25: privatemsg-572374.patch queued for re-testing.

Keeping track of this thread so that maybe I can help with testing...

#25: privatemsg-572374.patch queued for re-testing.

The patch still applies, those were just the broken roles tests which I've disabled for now.

I'll test this soon...

--Ben

I still didn't have the time to look in to the testing. I Have to find out how testing is working before I can write some testing rules. Looking at my schedule.... this will not going to happen soon :-(

Hey Berdir,

I tested the patch in #25 under a few different scenarios and everything worked well. So I think this is ready to be committed (unless you want to add some automated tests first). It's a nice feature. I'll help with testing the D7 version as soon as we have a port.

--Ben

Thanks for testing, this has waited long enough. Commited to 6.x-2.x.

I'll port this to D7 and then we can change this to a task to add tests for this feature.

Let's see if this passes.

Committed to 7.x-1.x.

Changing this to a task to add tests for this feature.

This issue is being closed because it is against a branch for a version of Drupal that is no longer supported.
If you feel that this issue is still valid, feel free to re-open and update it (and any possible patch) to work with the 7.x-1.x branch (bug fixes only) or the 7.x-2.x branch.
Thank you.