I'm almost positive an issue exists for this already, but I couldn't find it.

To reproduce:
1. Create some content as User A.
2. Log in as User B.
3. As User B, click on User A's name in the "Authored by" information of the node.
4. Notice that you are redirected to User B's user profile page, not User A's.

I imagine this is a one-line fix somewhere, but this fix should also come with tests to make sure that we don't accidentally introduce this again in the future.

Tagging as novice, since this should be a pretty easy test to write. There is some SimpleTest documentation at http://drupal.org/simpletest and sub-pages, although I find just opening up an existing .test file or two and reading it gives a pretty good introduction, too. If you have questions at all, come ask in #drupal! :)

CommentFileSizeAuthor
#9 theme-username-test.patch1.73 KBandypost
#4 user-profile-link-test3.patch1.8 KBdrifter
#2 user-profile-link-test2.patch1.79 KBdrifter
#1 user-profile-link-test.patch1.83 KBdrifter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drifter’s picture

drifter CreditAttribution: drifter commented
Assigned: Unassigned » drifter
Status: Active » Needs review
FileSize
user-profile-link-test.patch1.83 KB

Couldn't reproduce the bug on my fresh install, but here's my attempt at a test case. Two questions: should this belong in user.test or node.test? I opted for user.test because it tests for profile pages, but it could be either.

Second: I'm just testing for finding the "user/$user_a->uid" string in the raw HTML - I could have opted for matching the whole <a href="/user/$user_a->uid" title="View user profile."> tag, but it could make the test more fragile. Which is the better option? Or is there a better way than searching the full raw HTML?

drifter’s picture

drifter CreditAttribution: drifter commented
FileSize
user-profile-link-test2.patch1.79 KB

Cleaned up some comments and whitespace.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs review » Needs work

The patch contains some tab characters (starting around line 991). Can you re-roll the patch without the tabs (Coding standards)?

drifter’s picture

drifter CreditAttribution: drifter commented
Status: Needs work » Needs review
FileSize
user-profile-link-test3.patch1.8 KB

OK, rerolling with spaces instead of tabs...

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented

Looks good to me. Reviewed the logic and didn't find any issue with it. Applied the patch and ran the tests without problems.

Some minor remarks:

  • Non-documentation comments should use capitalized sentences with punctuation.
  • I'd drop the extra white-line at the start of testUserProfileLink()
  • Single quotes are recommended in favor of double-quotes, except for:
    1. In-line variable usage.
    2. Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes.

Other than the (cosmetic) issues above, I think this is good to go.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented

Regarding your question in #2: although your method looks ok to me (there shouldn't be false positives on "user/uid"), it might be better to add the a-tag and href-attribute to the mix. Here's an example from the Assertions handbook page:

$this->drupalGet('user/register');
$this->assertRaw('<a href="'. url('user/password') .'">', t('Make sure a link appears to "Request new password" on the user register page.'));
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I would highly suggest just using the following test:
$this->assertRaw(theme('username', $user_a), t("User A's profile link found."));

cburschka’s picture

cburschka
they
CreditAttribution: cburschka commented
Status: Needs review » Needs work

That sounds like a very good idea. The test should not hard-code the theme output it expects, I think.

(Naturally, this won't verify that theme('username', $user); actually works as it should - if theme('username', $user_b) themes a username link to the logged in user, then we don't catch that.)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
theme-username-test.patch1.73 KB

So here the patch

First check theme('username', ...) works then check result

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Postponed (maintainer needs more info)

I'm really confused why we need a test for this - there's no bug to be fixed, theme('username') depends on the information given to it. Marking 'needs more info' because I think we need a situation where this could actually happen - is it supposed to protect against completely wrong implementations of theme_username() which use global $user or something?

sun.core’s picture

sun.core CreditAttribution: sun.core commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)

I agree with catch.