Making it clear when users can and can't submit issues

hrm, i'm not sure how easy this would be to fix, since the issue add page is basically all handled via node.module, not project_issue.module. if you don't have permission to add a given node type, and you try to add one, it's node.module that's in control of the situation, and that generates the error.

so, probably the bug is that we're ever displaying an "add issue" link to a user that doesn't have permission to do so. *that* is something project_issue.module could do something about. and, in those cases, instead of printing a link to "submit bug" or whatever, it could print "Please login or register to submit a bug".

although, even authenticated users might not have permission to submit issues. so, just logging in might not be enough, and just because they're logged in doesn't mean they'd have permission. so, it'd have to be more complicated... we'd have to ensure the user had the right permission, and if not, figure out if they aren't in the right role, or aren't logged in, etc, and print a message accordingly.

or, probably the safest and simplest approach would be to only print the link at all if the user has perms, and otherwise, not print anything about how to add an issue. then, there's no confusion... if you have permission to do it, you see the link, otherwise, nothing...

anyway, this needs a little more thought before it'd be worth writing any code for it.

thanks for the report though, it's definitely worth fixing once we can come up with a reasonable plan.

cheers,
-derek

My apologies for semi-hijacking this issue, but dww's follow up is extremely relevant here and since there no longer seems to be a link to add an issue when not logged in I think the original issue has been fixed to some degree.

Currently it seems that if a user does not have permissions to create an issue there is no indication to the user that this is the case. I think this is bad UI, because for many sites (including d.o) allowing any random registered user the opportunity to submit a bug/feature request/issue is an important part of the development process.

Many of the core modules use something like a "Login or Register to ..." type message at the top of a content area (for example, in the forums). I think project issues should do something similar.

The best I can think of to do is something like:

• For anonymous users
  Login or Register to submit a bug, task, feature request, or support request. or maybe just
  Login or Register to submit an issue.
• For authenticated users
  You are not allowed to post a new bug, task, feature request, or support request. or
  You are not allowed to post a new issue.

These would both go on each project summary page, along with all of the links to the issue tracker. I think the individual issue pages should also get similar treatment with respect to the "Follow up" link.

The above is how forum.module handles this. It's still not ideal, but I think it at least makes it clear to an anonymous user that if they logged in or registered they would/might have permission to create an issue.

I'm planning on doing this for my site, so I have no problem submitting a patch if there's interest.

AC

The attached patch adds "Login or register" type text, similar to how this is done in comment.module

This is added in 2 places:
* On individual issue pages, "Login or register to follow up" is added in place of "Follow up"
* On the issue query result pages, "Submit" is replaced by "Login or register to create an issue"

I've created a similar issue at http://drupal.org/node/157769 that fixes the project summary pages as well and which depends on this patch.

AC

This, too, has whitespace badness and incomplete sentences for comments. :( After many hours of project* patch reviewing, I don't have the energy to re-roll this, so I'm just bumping back to needs work. ;)

Also, this seems weird:

+  if ($user->uid) {
+    return t("You are not allowed to create an issue");
+  }
+  else {
+    // we cannot use drupal_get_destination() because these links sometimes appear on /node and taxo listing pages
+    $destination = "destination=". drupal_urlencode("node/add/project_issue/$uri");  
+  }

Why put that particular code in the else clause, when the rest of the function assumes the same else? I'd rather just see this as:

+  if ($user->uid) {
+    return t("You are not allowed to create an issue");
+  }
+  // We cannot use drupal_get_destination() because these links sometimes appear on /node and taxo listing pages.
+  $destination = "destination=". drupal_urlencode("node/add/project_issue/$uri");  
+  ...

This, too, has whitespace badness and incomplete sentences for comments. :( After many hours of project* patch reviewing, I don't have the energy to re-roll this, so I'm just bumping back to needs work. ;)

The attached patch addresses these issues, I believe

Also, this seems weird:

+  if ($user->uid) {
+    return t("You are not allowed to create an issue");
+  }
+  else {
+    // we cannot use drupal_get_destination() because these links sometimes appear on /node and taxo listing pages
+    $destination = "destination=". drupal_urlencode("node/add/project_issue/$uri"); 
+  }

Why put that particular code in the else clause, when the rest of the function assumes the same else? I'd rather just see this as:

+  if ($user->uid) {
+    return t("You are not allowed to create an issue");
+  }
+  // We cannot use drupal_get_destination() because these links sometimes appear on /node and taxo listing pages.
+  $destination = "destination=". drupal_urlencode("node/add/project_issue/$uri"); 
+  ...

The way I had originally done it was copied directly from comment.module. I agree that it doesn't make a lot of sense, so I took out the else clause.

AC

i corrected some wonky indenting on that last patch.

thanks for the whitespace fix.

Visually the patch looks good now. Just tried it on a test site. I have mixed opinions...

In one sense, the basic goal is achieved, and that's a step forward.

However, there are a few things that don't seem right (though I don't have an answer for all of them just yet):

A) "You are not allowed to create an issue" looks out of place on the project/issues page for users without the perm. :( It looks like help text that's missing a period and formatting. It's also not clear what you're supposed to do to correct this error.

B) When you log out, the "Subscribe" link also disappears, but there's no equivalent "Login or register to subscribe" magic going on. Not sure we want that, but it does seem inconsistent.

C) If you're not logged in, and you use the "Login or register to follow up" link, and you log in as a user without perm to create issues, you land on an Access denied page. Since I didn't know, I just tried it and the way core handles this is that if you do the same thing for the "Login to post comments" link and log in as a user without perms to post comments, you get redirected to the node you were trying to comment on with a drupal_set_message(t('You are not authorized to post comments.'), 'error'). That seems much better.

D) Similar to (C) but for the "Login or register to create an issue" link -- this time, you go to node/add/project_issue but if you don't have perms, it's just the generic "Create content" page. :( If you were sent to node/add/project-issue, at least you'd get "Access denied", which is closer. Better yet would be a similar drupal_set_message() + redirect, but it's not clear where you're supposed to redirect, unless you put extra destination-related junk in the initial login link or something.

To conclude:
(A) I'm really not sure what to do about this, though I'm tempted to say it should just print no link at all if you're logged in and you don't have perms to create and issue or followup. I'm not sure I buy the argument in comment #2 that if you can't submit issues, it's bad UI not to tell you that. There's no precedent for that UI anywhere else in core that I know of. It's *definitely* bad UI to provide links that people can't use, but I'm not sure providing "There'd be a link here if you weren't a loser" text is an improvement. ;) Core doesn't print out "You cannot post comments" on every node that's comment-enabled for users that lack the perm.
(B) Not sure what to do here, either, but I guess we can leave it alone.
(C) Seems like a straight-forward fix.
(D) Seems like a can of worms.

So, I'd support a new patch that addressed (A) by ripping out the "You're a loser" text completely, fixed (C) as described above, and did something for (D), even if it's just to use the right link to hit the Access denied page. For extra credit, you'd try to fix (D) more like (C).

Thanks,
-Derek

(A) I'm really not sure what to do about this, though I'm tempted to say it should just print no link at all if you're logged in and you don't have perms to create and issue or followup.

I thought it might be best to so the same thing that's done elsewhere in core, but I agree that it does look strange to do this. I have no problem just not printing anything if the user IS logged in but does NOT have permission to submit an issue or post a follow up.

I'm not sure I buy the argument in comment #2 that if you can't submit issues, it's bad UI not to tell you that.

I don't think I was clear about what I meant. I feel that if a user is NOT logged in it is a good idea to inform them that if they logged in they (might) could submit an issue, etc. But if they are logged in and we know they DON'T have permission, then I don't see the need to remind them of how much of a loser they are!

There's no precedent for that UI anywhere else in core that I know of. It's *definitely* bad UI to provide links that people can't use, but I'm not sure providing "There'd be a link here if you weren't a loser" text is an improvement. ;) Core doesn't print out "You cannot post comments" on every node that's comment-enabled for users that lack the perm.

Actually, on my test site, for both comments and forums, a user who is logged in but who does not have "post comments" or "create forum topics" permission is explicitly informed of this just as in the patch I provided above. As far as I can tell this is not due to some customization I have since forgotten. This might not show up on d.o because I think all authenticated users have permission to make forum posts, comment on a node (if comments are allowed at all for the node type), and submit issues.

But I certainly agree that it's a bad idea to provide links that aren't functional to a user if the code knows that already.

B) When you log out, the "Subscribe" link also disappears, but there's no equivalent "Login or register to subscribe" magic going on. Not sure we want that, but it does seem inconsistent.

True. I've changed this to "Login or register to create or subscribe to an issue" on the list of issues page.

C) If you're not logged in, and you use the "Login or register to follow up" link, and you log in as a user without perm to create issues, you land on an Access denied page. Since I didn't know, I just tried it and the way core handles this is that if you do the same thing for the "Login to post comments" link and log in as a user without perms to post comments, you get redirected to the node you were trying to comment on with a drupal_set_message(t('You are not authorized to post comments.'), 'error'). That seems much better.

I hadn't noticed that behavior from comment before, but that's pretty cool. However, implementing this wasn't as easy as it sounds.

I had to change the callback to project_comment_page() in project_issue.module and had to change project_comment_page() to do the access checking and to send the user back to the project_issue node with a message if they don't have permission to post issues.

D) Similar to (C) but for the "Login or register to create an issue" link -- this time, you go to node/add/project_issue but if you don't have perms, it's just the generic "Create content" page. :( If you were sent to node/add/project-issue, at least you'd get "Access denied", which is closer. Better yet would be a similar drupal_set_message() + redirect, but it's not clear where you're supposed to redirect, unless you put extra destination-related junk in the initial login link or something.

I changed the redirection to node/add/project-issue, and it seems like this is the way that other node types behave. I don't think it's really possible to redirect the user elsewhere, because the redirection to either "Access denied" or the content types page happens in node_add(). It might be possible to make some sort of wrapper menu callback that would redirect a user back to the project overview page or the project issues page, but I don't see the need to do that. No extra credit for me today :)

So, the attached patch fixes A, B, and C, and addresses the primary issue in D. I tested it on my site, but as I've never really dealt with the menu API before, it should be closely reviewed to make sure I didn't do anything wrong in my changes to project_issue_menu().

I'll leave http://drupal.org/node/157769 as status=postponed until I get feedback on this patch.

Excellent, almost there. However, I think the "Login or register to create or subscribe" thing is worse than just leaving subscribe completely out of it. If you click on the "login" link, you get redirected immediately to the create form, not the subscribe form, and it's actually quite hard to find your way back to the subscribe link from there. :(

So, either we need:

1) Another set of theme functions for the subscribe link.

2) To modify the existing ones to take an $op argument and behave accordingly.

3) Forget about "subscribe" entirely for the purposes of this patch, and just let that link magically appear once you log in, but don't bother telling anonymous users about it (at least not for now).

Subscribe needs a lot of help, anyway, so I'd be ok with (3) for now and addressing the usability of the subscribe interface another day.

If you choose (1) or (2), don't forget that there's a separate call to create the link on project/issues vs. project/issues/[foo]. Your patch has the "... or subscibe" for a specific project, but not on the all projects page.

Finally, this comment:

-        // Only add the link if we're not already on an issue follow-up.

Is specific to this code:

           !(arg(0) == 'project' && arg(1) == 'comments')) {

You moved it into the case for this:

+        if (user_access('create project issues')) {

That's misleading. ;) Please leave that comment next to the code it's talking about.

Thanks!
-Derek

Also, sorry, I just committed http://drupal.org/node/159544, which is going to partially break this patch (though your new text is already compliant with "create" instead of "submit"). But, since this needed re-rolling anyway, I figured it was safer to commit that first and let you post a new patch that applies cleanly to HEAD. Sorry for the trouble...

ok, I think this patch should do it.

Not quite. ;) Your latest patch:

A) Left out the changes to comment.inc, which are quite important.
B) Still moved the code comment in the unhelpful way I mentioned in my previous reply.

However, you've already spent a lot of time on this issue, so I fixed those myself, and committed to HEAD. For future reference, I'm attaching the patch I actually committed (not include the update to the translation template, which I also committed at the same time).

This patch doesn't apply to DRUPAL-4-7--2 or DRUPAL-4-7, I don't think I'll backport it. This isn't *really* a bug, IMHO, since at least we weren't printing links you couldn't use, only not telling people if they logged in they might be able to create/follow-up to issues....

Anyway, thanks for all the effort on this, it's a nice touch, and will be slick for drupal.org as soon as I upgrade the site.

Sorry about that. I saw the part in comment.inc that I deleted manually from the patch because I got confused and thought it was from something else. I had to run home before they started filming Batman on my route home, and didn't get as much time to test it as I wanted. Thanks for making it pretty (and functional :) ).