Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Contact module has wrong access expression for displaying Contact tab in user account. When you have "administer users" permissions, you can see Contact tab in your own profile. But it's not reasonable, as even if you admin, it doesn't make sense to contact yourself.
Here are two patches fro D7 and D6 to fix that usability issue.
Comment | File | Size | Author |
---|---|---|---|
#69 | After-patch-contact.png | 184.25 KB | Madhu kumar |
#69 | before-patch-contact.png | 171.43 KB | Madhu kumar |
#65 | 602234-after_patch.png | 42.48 KB | Abhijith S |
#65 | 602234-before_patch.png | 49.5 KB | Abhijith S |
#64 | After.png | 79.07 KB | anmolgoyal74 |
Comments
Comment #2
neochief CreditAttribution: neochief commentedComment #3
jim0203 CreditAttribution: jim0203 commentedDoes what it says on the tin and, barring some crazy circumstance where someone would want to contact themselves, a good move. This is for the D7 patch btw; haven't looked at the D6 one.
Comment #4
neochief CreditAttribution: neochief commentedyeah, crazy circumstance = dual personality :)
Comment #5
dcor CreditAttribution: dcor commentedreviewing
Comment #6
dcor CreditAttribution: dcor commentedNo problems with patches.
Works like a charm in D6... you have my stamp of approval. "Contact" tab is there under regular circumstances, apply the patch and "contact" tab disappears for my user but is still there for other users. Tried it in D7 too.
Attached are screen shots of d6..
Comment #7
dcor CreditAttribution: dcor commentedunassign
Comment #8
neochief CreditAttribution: neochief commentedthanks for review!
Comment #9
Dave ReidPlease include a simple test to make sure this stays fixed for both normal users and user-admin users.
Comment #10
Dave ReidComment #11
gpk CreditAttribution: gpk commentedIs is this simple? Since now in 7.x you can give anonymous users permission to view user contact forms, it would be odd if you can only view your own contact form if you are logged out.
See also #345541: Link to contact form in user account and e-mails leads to 403..
Comment #12
skirpichev CreditAttribution: skirpichev commentedsubscribe
Comment #13
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedPatch based on #2 with tests for review.
Comment #15
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedRe-rolled patch from #13.
Comment #16
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTested the patch in #15 with success. With the patch the contact page of user 1 is reachable as user 1, without it's not reachable (see attached screenshots 1 and 2). The contact form of user 1 is still accessible as an anonymous user with the "Use users' personal contact forms" permission (see attached screenshot 3).
I reviewed the patch and the code looks good and has the necessary tests so this looks RTBC to me. It basically just prevents the personal contact form tab access check from returning TRUE if the visitor tries to view his/her own contact form, while preserving the administrators' rights to view all users' personal contact forms. The "can not view own contact form" check was already there before but was not in the right order, leaving administrators with the possibility of viewing their own contact form.
Comment #17
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThis was committed to D8: a478bad.
Comment #18
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedHere are the patches for 7.x.
Comment #19
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedClean backport :)
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedI'm not sure this makes sense - if I'm an administrator who just turned on the Contact module, one of the first things I'd probably do is go to my own account and see if the tab is there (and probably try to contact myself also to test how it works).
I ran "git blame" to see when this code was originally added, and after a long slog through Git history I came across #70895: usability: don't show contact when viewing *own* profile. Reading the comments there, leaving the tab there for administrators was indeed done on purpose (and for the reason I mentioned above).
So personally I think this should be rolled back in Drupal 8. But maybe either way we need better code comments here to explain the logic of why the code does what it does.
Comment #21
andypostThis needs re-roll for D8
Comment #22
krishworks CreditAttribution: krishworks commentedthe code discussed in 7.x patch is already in 8.x. See below taken from 8.x code as of today.
Comment #23
talhaparacha CreditAttribution: talhaparacha as a volunteer commented@David_Rothstein has rightly pointed in #20 that support for this functionality was intentionally added. But by looking at the code, it can be seen that the "can not view own contact form" check was also already there but was not in the right order, as explained in #16. So I think we have a bit of dilemma here and we need to make a decision now on whether the functionality should be removed or not.
If we go with removing this functionality (as was the original intent of this issue), then we can simply RTBC this issue. The fix for D8 has been committed. And I just tested that the patches from #18 still apply cleanly to D7 (commit # 1990861)
If we go with keeping this functionality, then we need to re-roll what got committed to D8 in #17. A patch is here in this regard.
In either case, the issue needs more review.
Comment #26
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedAccidentally got the patch tested for D7. Though I don't know why the previous patch failed for D8. But lets make a decision first on what to do with this issue before working on the patch again...
Comment #27
andyposttests should be fixed, not removed
Comment #28
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedFixed the test instead of removing it as per #27. Also, trying to fix what got the tests to fail in #23.
Again, the patch is intended to re-roll what got committed in #17 as per the suggestion in #20.
Comment #31
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedThis should pass the tests...
Comment #34
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedBecause retesting the patch went successful...
Comment #35
andypostComment #36
surbz CreditAttribution: surbz at Srijan | A Material+ Company commentedHello, I installed drupal 8.2.x-dev and could not replicate the above issue.
Screen shot attached.
I also tried it with drupal 7.x-dev and could replicate it.
Comment #37
andypostComment #51
shaktikCreate a patch for 9.1 and tested on my local working fine, kindly check.
Comment #52
samiullah CreditAttribution: samiullah at Salsa Digital commentedTested the patch on local clean install of drupal 9.1x
1. Before applying patch contact link is not visible in user profile
2. After applying patch contact link is visible
3. Anonymous user get access denied if they access particular users form example /user/1/contact
4. if authenticated user tried to access different users contact form he gets page not found example if user with uid 1 tries to access form of user with id 3 /user/3/contact
Looks good, waiting for tests to PASS
This can be moved to RTBC if code review is also fine
Comment #53
mayurjadhav CreditAttribution: mayurjadhav at Srijan | A Material+ Company commentedPatch applied cleanly on drupal 9.1x, Patch looks good, +1 for RTBC.
Comment #54
samiullah CreditAttribution: samiullah at Salsa Digital commentedComment #56
larowlanLet's not use gendered language here - their own was in the original comment, let's retain that
It's not clear what the remaining tasks of this issue are, it was committed in Mar 2020, but there's still a patch.
Can we get an issue summary update reflecting why the issue is still open? Thanks.
Comment #57
larowlanComment #58
shaktikHi @larowlan,
Just updated for
, kindly check.
Thanks,
Shakti.
Comment #59
samiullah CreditAttribution: samiullah at Salsa Digital commentedComment change looks good, waiting for review from @larowlan
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedSetting NW for the issue summary update requested in #56.
Personally, I think 'including their own' is easier to read.
Comment #62
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAgree with #60
updated patch as suggested in #60.
Comment #63
quietone CreditAttribution: quietone as a volunteer commented@adityasingh, thanks for the reroll.
This needs an issue summary update, requested in #56 and #62. To assist that process I have added the issue summary template and trimmed it to only those sections that need to be finished before this can be marked RTBC. The general guidelines for summaries provides guidance on what needs to be added. This work is suitable for a novice.
Oh, up to date screenshots are needed, I've marked a place in the issue summary where they can be added. I don't know if the screen shots from #52 are up to date. I haven't looked at the changes to the patch.
Comment #64
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAdding screenshots.
Comment #65
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #62 and it works fine.
Before patch:
After patch:
Comment #66
guilhermevp CreditAttribution: guilhermevp at CI&T commentedI was unable to reproduce the error in in 9.2.x. As far I understood the point of the issue is to be unable to contact yourself, and as it is in 9.2.x seems impossible to do so. Maybe I'm doing something wrong. I have interest in filling the summary update but I could use steps to recreate the issue.
Comment #67
guilhermevp CreditAttribution: guilhermevp at CI&T commentedComment #68
quietone CreditAttribution: quietone as a volunteer commented@guilhermevp, Welcome to Drupal. I too am finding this a bit confusing and like you I find that this is already fixed.
So what needs to be done? I read through the issue and found the answer in, #20. It points out that this overrides decisions made in an earlier patch. That history should be in the Issue Summary, enough so reviewers understand the problem. And in the 'remaining tasks' to state the next step, like 'Decide if the ability for the admin to contact themselves should be removed' and thus remove the capabilities pointed to in the issue referred to in #20.
@guilhermevp, would you like to work on the Issue Summary update?
Comment #69
Madhu kumar CreditAttribution: Madhu kumar as a volunteer and at Zyxware Technologies commented@guilhermevp I was able to find the error in in 9.2.x. Added screenshot for the reference.
patch #62 applied cleanly and working as expected.
Comment #70
guilhermevp CreditAttribution: guilhermevp at CI&T commented@quietone
I dabbled in making a summary update. Let me know what you think.
Comment #71
guilhermevp CreditAttribution: guilhermevp at CI&T commentedComment #72
quietone CreditAttribution: quietone as a volunteer commented@guilhermevp, Very nice update. Good job! To further improve it this string "who just turned" can be changed to "who had just turned" and instead of the link with the text, 'this issue', change it to
which will expand it to show the issue number and issue title.
Putting on my reviewers hat, I think this can be further improved to make it easier for a reviewer. The first thing I notice is the title. Is it accurate? I don't think so because not we are deciding whether to have the tab or not. Maybe change it to 'Should the Contact yourself tab be in the user profile for the admin'. What do you think?
The next thing it would be helpful to know is that this issue was committed. Since it is not so common that an issue continues after commit (and not a backport) let's emphasize that. In the fist section, problem/motivation add a beginning paragraph. Something like this "This issue was committed in commit (put the hash link here) and then in #20 David_Rothstein pointed out that the behavior was a deliberate choice in #70895: usability: don't show contact when viewing *own* profile. The issue is currently to discuss whether to restore the previous behavior or not."
Then, there are a lot of blank sections. As a reviewer I don't know if these are to be filled out later or not. So for 'steps to reproduce' I search the issue to see if is they are available in a comment and then copy/paste. If not, either add it myself or add TBA (to be added). In this case, 'TBA' is fine. And in 'Proposed resolution' I would add the two choices.
And then, to finish off the sections, in 'user interface changes' add 'TBD'.
@guilhermevp, looking forward to your changes!
Comment #73
quietone CreditAttribution: quietone as a volunteer commented@Madhu kumar, Remember to read the issue before adding a comment. The screenshots you have added were already added in #65.
Comment #74
guilhermevp CreditAttribution: guilhermevp at CI&T commented@quietone
Thanks for the feeback, I tried to change things a bit from your suggestions, but most of it was just perfect summarization of the updates the issue needed. Again, thanks!
Comment #75
guilhermevp CreditAttribution: guilhermevp at CI&T commentedComment #77
pameeela CreditAttribution: pameeela commentedCan someone who has worked on this lately update the issue summary with steps to reproduce the issue identified (this makes it easier to test that it is fixed) as well as the details of the proposed approach?
Comment #78
bnjmnmI recommend closing this issue and creating a new one that references it. Even if the issue summary is cleaned up, the comment history will more likely confuse than it will help. That commit history is bizarre/
@guilhermevp did a great job updating the issue summary, and some of his beneficial changes have already been reverted. This is a very old issue that started in 7.x. A new issue can repeat the valuable parts from here, but remove the noise.
I think the issue title of "Should the Admin be able to contact themself in the user profile" that @guilhermevp is the right approach. We need to determine if that is an acceptable change before anything is altered. This is still being interpreted as a bug when it isn't a bug. It's currently expected behavior (that I wouldn't mind seeing changed 🙂).
If you don't agree, remove the "needs followup" tag, but I think it will vastly increase the likelihood of contributors understanding what is going on.
Comment #79
pameeela CreditAttribution: pameeela commentedI agree, this is really confusing. Also agree it's not a bug since it was an intentional change. The only problem with closing this to create a new issue is there is a valid patch.
I've created #3221619: Allow users with 'Administer users' permission to use their own contact form. @adityasingh you can upload your patch to that issue but I wouldn't mark it Needs Review until there is consensus that the change should be made.
I'm also updating the issue back to the original title and summary so that it's clear what was actually changed here.
Comment #80
bnjmnmThanks @pameeela! I updated [##3221619] with the patch from here, documented that the contributors from this issue should receive credit if it lands, and tagged it to hopefully get product manager approval on making a users own contact form visible.
Comment #81
pameeela CreditAttribution: pameeela commentedI believe this should be marked as ‘Fixed’ since it was committed to 8.x way back when. But now I’m not sure because there’s just a comment linking to a commit so maybe it was committed via another issue.
Comment #82
quietone CreditAttribution: quietone as a volunteer commented@bnjmnm and @pameeela, thanks for sorting this out. It has been in my too hard basket for a while now.
I can confirm this was committed. changing fixed. The followup has been created, removing tag.