Skip to content

Commit

Permalink
[User Accounts] Hide UI option allowing users to edit their own appro…
Browse files Browse the repository at this point in the history 
…val status (#5353)

Fix #4743 where a user could accidentally lock themselves out but editing their own approval status.
  • Loading branch information
@johnsaigle @driusan
johnsaigle authored and driusan committed Nov 20, 2019
1 parent 966c242 commit efda4ff
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 48 deletions.
103 changes: 58 additions & 45 deletions modules/user_accounts/php/edit_user.class.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -812,18 +812,23 @@ class Edit_User extends \NDB_Form
'N' => 'No',
)
);
$groupB[] = $this->createLabel(
"Pending Approval:"
);
$groupB[] = $this->createSelect(
"examiner_pending",
"Pending Approval: ",
array(
null => "",
'Y' => 'Yes',
'N' => 'No',
)
);

// A user should not be able to edit their own examiner approval
// status.
if (!$this->_isEditingOwnAccount()) {
$groupB[] = $this->createLabel(
"Pending Approval:"
);
$groupB[] = $this->createSelect(
"examiner_pending",
"Pending Approval: ",
array(
null => "",
'Y' => 'Yes',
'N' => 'No',
)
);
}
$this->addGroup(
$groupA,
'examiner_sites',
Expand All @@ -842,42 +847,50 @@ class Edit_User extends \NDB_Form
unset($groupB);
}

// active
$this->addSelect('Active', 'Active', array('Y' => 'Yes', 'N' => 'No'));
$this->addSelect(
'Pending_approval',
'Pending approval',
array(
'Y' => 'Yes',
'N' => 'No',
)
);

//------------------------------------------------------------
// It doesn't make sense for a user to be editing their own
// approval status or active status. In fact this could result in a
// Darwinian self-lockout. Let us help those who cannot help themselves.
if (!$this->_isEditingOwnAccount()) {
$this->addSelect(
'Pending_approval',
'Pending approval',
array(
'Y' => 'Yes',
'N' => 'No',
)
);

// active time windows
$this->addSelect(
'Active',
'Active',
array(
'Y' => 'Yes',
'N' => 'No'
)
);

$dateOptions = array(
'language' => 'en',
'format' => 'YMd',
'addEmptyOption' => 'true',
);
$dateOptions = array(
'language' => 'en',
'format' => 'YMd',
'addEmptyOption' => 'true',
);

$dateAttributes = ['class' => 'form-control input-sm input-date'];
$dateAttributes = ['class' => 'form-control input-sm input-date'];

$this->addBasicDate(
'active_from',
'Active from',
$dateOptions,
$dateAttributes
);
$this->addBasicDate(
'active_from',
'Active from',
$dateOptions,
$dateAttributes
);

$this->addBasicDate(
'active_to',
'Active to',
$dateOptions,
$dateAttributes
);
$this->addBasicDate(
'active_to',
'Active to',
$dateOptions,
$dateAttributes
);
}

// checking if the account can be rejected (no password hash
// pending approval)
Expand Down Expand Up @@ -1204,8 +1217,8 @@ class Edit_User extends \NDB_Form
}
}
if (!$matched
&& (($values['examiner_radiologist'] !== ''
|| $values['examiner_pending'] !== ''))
&& ($values['examiner_radiologist'] !== ''
|| $values['examiner_pending'] ?? '' !== '')
) {
$errors['examiner_sites'] = "Please select at least one examiner
site or clear the 'Examiner status' fields below (i.e.
Expand Down
57 changes: 54 additions & 3 deletions modules/user_accounts/test/user_accountsTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,53 @@ class UserAccountsIntegrationTest extends LorisIntegrationTest
private $_table = "#dynamictable > tbody > tr:nth-child(1)";
private $_addUserBtn = "#default-panel > div > div > div.table-header >".
" div > div > div:nth-child(2) > button:nth-child(1)";

/**
* Does basic setting up of Loris variables for this test, such as
* instantiting the config and database objects, creating a user
* to user for the tests, and logging in.
*
* @return void
*/
public function setUp()
{
parent::setUp();
$password = new \Password($this->validPassword);
$this->DB->insert(
"users",
array(
'ID' => 999995,
'UserID' => 'UnitTesterTwo',
'Real_name' => 'Unit Tester 2',
'First_name' => 'Unit 2',
'Last_name' => 'Tester 2',
'Email' => '[email protected]',
'Privilege' => 0,
'PSCPI' => 'N',
'Active' => 'Y',
'Password_hash' => $password,
'Password_expiry' => '2099-12-31',
'Pending_approval' => 'N',
)
);

$this->DB->insert(
"user_psc_rel",
array(
'UserID' => 999995,
'CenterID' => 1,
)
);

$this->DB->insert(
"user_project_rel",
array(
'UserID' => 999995,
'ProjectID' => 1,
)
);
}

/**
* Tests that, when loading the User accounts module > edit_user submodule, some
* text appears in the body.
Expand Down Expand Up @@ -119,7 +166,6 @@ function testUserAccountsMyPreferencesDoespageLoad()
function testUserAccountsFilterClearBtn()
{
$this->safeGet($this->url . "/user_accounts/");
//testing data from RBdata.sql
$this-> _testFilter($this->_name, $this->_table, null, "UnitTester");
$this-> _testFilter($this->_site, $this->_table, "1 rows", "3");
}
Expand Down Expand Up @@ -202,7 +248,7 @@ function testUserAccountEdits()
// Test changing 'Active' status
$this->_verifyUserModification(
'user_accounts',
'UnitTester',
'UnitTesterTwo',
'Active',
'No'
);
Expand All @@ -216,10 +262,12 @@ function testUserAccountEdits()
// Test changing Approval status
$this->_verifyUserModification(
'user_accounts',
'UnitTester',
'UnitTesterTwo',
'Pending_approval',
'No'
);
//TODO:add test case to ensure pending_approval
//DOES NOT show up on UnitTester since logged in user is UnitTester
}
/**
* Tests various My Preference page edit operations.
Expand Down Expand Up @@ -585,6 +633,9 @@ function _accessUser($page, $userId)
function tearDown()
{
$this->DB->delete("users", array("UserID" => 'userid'));
$this->DB->delete("user_psc_rel", array("UserID" => 999995));
$this->DB->delete("user_project_rel", array("UserID" => 999995));
$this->DB->delete("users", array("UserID" => 'UnitTesterTwo'));
parent::tearDown();
}
}
Expand Down

0 comments on commit efda4ff

Please sign in to comment.