The module tests fail. In the specific, it's testPasswordReset() to fail, with the consequence that all the automatic tests ran when a patch is submitted in a issue don't run, and keep waiting for the branch tests to pass.

I am setting the priority to Major, since this is a problem for many patches provided for this module.

Comments

kiamlaluno created an issue. See original summary.

Rafal Lukawiecki’s picture

This is caused by the unnecessary, in my opinion, assert of the redirection path. That assert no longer works due to the outdated "?q=" fragment. I have bypassed that in the patch attached to https://www.drupal.org/project/legal/issues/2504177#comment-13737088. That patch passes with that assert removed.

You can see the value expected and the value returned in that assert in this test run: https://www.drupal.org/pift-ci-job/1750552 which shows:

fail: [Other] Line 156 of sites/all/modules/legal/legal.test:
Value '/' is equal to value '/?q=user/3/edit'.

If there is interest, I would be happy to create a patch just for the branch and this test—please let me know.

avpaderno’s picture

The only test I see checking that is testPasswordReset(), which uses the following code.

    // Create a log in link for the user, and go to that URL.
    // Borrowed from testUserPasswordResetExpired().
    $timestamp = REQUEST_TIME;
    $this->drupalGet("user/reset/{$this->user->uid}/$timestamp/" . user_pass_rehash($this->user->pass, $timestamp, $this->user->login, $this->user->uid));

    $this->assertText(t('Reset password'), "The reset password form is shown.");

    // Use the one-time login link.
    $this->drupalPost(NULL, array(), t('Log in'));

    $this->assertTitle(t('Terms and Conditions | Drupal'), "The user is redirected to the terms and conditions approval page.");
    $this->assertText(t('Terms and Conditions'), "The Terms and Conditions form is shown.");
    // assertText() doesn't handle an '&' properly.
    $this->assertText(t('To continue to use this site please read the Terms & Conditions below'), "The Terms and Conditions form is shown.");

    $edit = array(
      'legal_accept' => TRUE,
    );

    // Accept the T&Cs.
    $this->drupalPost(NULL, $edit, t('Confirm'));

    // Don't use assertUrl() as that requires us to match up the tokens in the
    // query too.
    $url = $this->getUrl();
    $path = parse_url($url, PHP_URL_PATH);
    $expected_path = url("user/{$this->user->uid}/edit");
    $this->assertEqual($path, $expected_path, "The user is redirected to the user edit page.");

    $this->assertText(t("You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password."), "The user is shown the message to reset their password.");
  }

What seems wrong is the code for getting the current page path.
I am not sure it's necessary to verify the user sees the form to edit the account. The test already verifies the message shown to users using their one-time login link is present; that should be enough.

Rafal Lukawiecki’s picture

StatusFileSize
new812 bytes

I agree. This section of the code should be removed:

$url = $this->getUrl();
    $path = parse_url($url, PHP_URL_PATH);
    $expected_path = url("user/{$this->user->uid}/edit");
    $this->assertEqual($path, $expected_path, "The user is redirected to the user edit page.");

I have already removed it in my patch for #2504177: Avoid redirecting to user page after legal acceptance and it passed the tests.

For what it is worth, the test fails because $path="/" while $expected_path="/?q=user/3/edit" during the test run, which is caused by incorrect parsing of the URL (fragment vs path) which is likely to change dependent on how Drupal is installed.

I attach a patch that removes this section, making the branch testable again.

Rafal Lukawiecki’s picture

Status: Active » Needs review
avpaderno’s picture

To make a comparison, this is the code used from UserPasswordResetTestCase::testUserPasswordResetLoggedIn().

  /**
   * Test user password reset while logged in.
   */
  function testUserPasswordResetLoggedIn() {
    $account = $this->drupalCreateUser();
    $this->drupalLogin($account);
    // Make sure the test account has a valid password.
    user_save($account, array('pass' => user_password()));

    // Generate one time login link.
    $reset_url = user_pass_reset_url($account);
    $this->drupalGet($reset_url);

    $this->assertText('Reset password');
    $this->drupalPost(NULL, NULL, t('Log in'));

    $this->assertText('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.');

    $pass = user_password();
    $edit = array(
      'pass[pass1]' => $pass,
      'pass[pass2]' => $pass,
    );
    $this->drupalPost(NULL, $edit, t('Save'));

    $this->assertText('The changes have been saved.');
  }

It doesn't check on which page the users are redirect, but only if the You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password. message is shown on the page.

Definitively, the test of this module should just verify the message is present, without checking where the users are redirect.

avpaderno’s picture

Status: Needs review » Reviewed & tested by the community

Tests are passing. This is R&TBC.

@Rafal Lukawiecki Good work on finding the cause and fixing it!

Rafal Lukawiecki’s picture

Thank you for your kind words and your help with this issue, @kiamlaluno.

robert castelo’s picture

Status: Reviewed & tested by the community » Fixed

Thanks to everyone who helped get this issue fixed.

Status: Fixed » Closed (fixed)

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