Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[user_accounts] Registering a user as an examiner #2190

Merged
merged 17 commits into from
Oct 18, 2016

Conversation

ridz1208
Copy link
Collaborator

add support for registering a user as an examiner from the user accounts edit user menu

@ridz1208 ridz1208 added Feature PR or issue introducing/requiring at least one new feature Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Sep 15, 2016
@ridz1208 ridz1208 added this to the 17.0 milestone Sep 15, 2016
@codecov-io
Copy link

codecov-io commented Sep 15, 2016

Current coverage is 13.68% (diff: 5.75%)

Merging #2190 into 17.0-dev will decrease coverage by 0.08%

@@           17.0-dev      #2190   diff @@
==========================================
  Files           121        121          
  Lines         20241      20380   +139   
  Methods        1132       1133     +1   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2787       2788     +1   
- Misses        17454      17592   +138   
  Partials          0          0          

Powered by Codecov. Last update 096d08a...6963c0d

@@ -143,6 +144,28 @@
'Email' => $from,
'CenterID' => $site,
);
/*
foreach($_REQUEST as $k=>$v) {
error_log($k." --> ".$v);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ridz1208
user_accounts/test/user_accountsTest.php:184
$this->_verifyUserModification(
'user_accounts',
'UnitTester',
'First_name',
// 'NewFirst' => 'Unit',
'Unit'
);
try it

@ridz1208
Copy link
Collaborator Author

@taracampbell try entire procedure as new user requesting account plz

@gluneau try messing around with front end while verifying database changes in effect and code review please

@LeighMac, @driusan satisfied ???

@ridz1208 ridz1208 added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) SQL PR contains SQL modifications such as schema changes and new SQL scripts and removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Sep 16, 2016
@kongtiaowang kongtiaowang added the Passed Manual Tests PR has undergone proper testing by at least one peer label Oct 6, 2016
@ridz1208 ridz1208 changed the base branch from 17.0-dev to 16.1-dev October 7, 2016 16:54
@ridz1208 ridz1208 changed the base branch from 16.1-dev to 17.0-dev October 7, 2016 16:54
@ridz1208 ridz1208 force-pushed the 2016_09_14_user_accounts_add_examiner branch from 6963c0d to 00e98f5 Compare October 7, 2016 17:00
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no label beside the examiner_radiologist and examiner_pending dropdowns in 'Examiner Status' group

);
}
$groupB[] = $this->createLabel(
"&nbsp;&nbsp;&nbsp;&nbsp; <b>Radiologist: </b>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sur about the html in php. This should be in the template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML in PHP is done throughout this module (see 670-678) and maintained here for consistency of the template form

- {$form.examiner_sites.label} - -
- {$form.examiner_sites.html} -

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

)
);
$groupB[] = $this->createLabel(
"&nbsp;&nbsp;&nbsp;&nbsp; <b>Pending Approval: </b>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sur about the html in php. This should be in the template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@xlecours
Copy link
Contributor

Test cases to be added in the test plan if not in the Integration tests.

@ridz1208
Copy link
Collaborator Author

@xlecours
screenshot from 2016-10-12 12 10 25

@taracampbell
Copy link
Contributor

@ridz1208, I just tested. After requesting an account, that user gets added to the examiner table successfully. However, there appears to be a problem with adding them to the user table (i.e. they do not get added, the name does not show up in user accounts).

I get errors like this in my error log:

PDOStatement::execute(): SQLSTATE[23000]: Integrity constraint violation: 1452 Cannot add or update a child row: a foreign key constraint fails (`ccna`.`history`, CONSTRAINT `FK_history_1` FOREIGN KEY (`userID`) REFERENCES `users` (`UserID`)) in /Users/Tara/Sites/loris/php/libraries/Database.class.inc on line 914, referer: http://sandbox.loris.com/request_account/process_new_account.php

Perhaps this is an unrelated issue though.

@ridz1208
Copy link
Collaborator Author

@taracampbell that seems to be a ccna specific issue try adding a user called lorisadmin to your users table. See if that solves it https://github.com/aces/CCNA/pull/737

@@ -909,6 +909,7 @@ class NDB_BVL_Instrument extends NDB_Page
WHERE testID =:tid
AND pass =:cert_id
AND centerID =:cid
AND examiners.active='Y'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here to existing data if there's data already saved and the examiner active flag changes?

@taracampbell
Copy link
Contributor

@ridz1208 I already have the lorisadmin user, so I don't think that's problem. I will try to figure out what's going on, but if someone has already tested this and it is working, I don't see why my additional validation is required in order to merge this.

@taracampbell
Copy link
Contributor

Basic testing done - everything seems to be working.

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obviously wrong with this.

@driusan driusan merged commit 73df96f into aces:17.0-dev Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Feature PR or issue introducing/requiring at least one new feature Passed Manual Tests PR has undergone proper testing by at least one peer SQL PR contains SQL modifications such as schema changes and new SQL scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants