-
Notifications
You must be signed in to change notification settings - Fork 1
HEEDLS-548 Exclude centres from self registration #501
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change itself looks fine, can we just add a small unit test to for this case in the same pattern as Get_active_centres_should_not_contain_an_inactive_centre
|
@AlexJacksonDS Thanks! I added a unit test, let me know if it looks okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
9e30922 to
be7d3bf
Compare
| @"SELECT CentreID, CentreName | ||
| FROM Centres | ||
| WHERE Active = 1 | ||
| AND kbSelfRegister = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to put this in the db query level given that this is only used in registration, but I think we should modify the name of the method so it's clear that we're getting the centre options for delegate self registration (rather than a true active centres list, which should technically only have the Active = 1 constraint)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I changed the method name to account for this
be7d3bf to
994c16c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'd probably remove the "Active" from the method name as it's unnecessary with the extra specification we've added, but no need for rereview 👍
Small fix - I added one line to exclude centres with kbSelfRegister=0 from the select options in the delegate self-registration journey.