-
Notifications
You must be signed in to change notification settings - Fork 4
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
ER-1377_show link to restore access for blocked provider. #810
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.
Looks good. I think certain bits need renaming so they dovetail nicely with with the existing blocking functionality.
@@ -7,6 +7,7 @@ public static class RoutePaths | |||
public const string ExceptionHandlingPath = "/error/handle"; | |||
public const string BlockedOrganisations = "/blockedorganisations"; | |||
public const string WithdrawVacancyPath = "/withdraw"; | |||
public const string UnBlockOrganisationRoutePath = "/restoreaccess"; |
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.
Shouldn't everything to do with blocking/unblocking a provider come off of blockedorganisations
. I don't think we need to add a new routepath
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.
👍
{ | ||
if (ModelState.IsValid == false) | ||
{ | ||
return View(new UnBlockTrainingProviderEditModel() { Ukprn = model.Ukprn, ProviderName = model.ProviderName }); |
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.
Naming is a bit wonky between action names, view models and views. Try and keep them aligned if possible.
Can we change usages of the word RestoreAccess
to Unblock
. The front-end can say 'Restore Access' but I think in the code keeping to block/unblock is easier to understand.
Change the ViewModel from UnBlockTrainingProviderEditModel
to ConfirmTrainingProviderUnblockingViewModel
so it aligns with the existing ConfirmTrainingProviderBlockingViewModel
.
Change this Function name to ConfirmTrainingProviderUnblocking
Change the view from RestoreAccess.cshtml
to ConfirmTrainingProviderUnblocking.cshtml
.
...etc
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.
👍
var isBlocked = await _orchestrator.IsProviderAlreadyBlocked(model.Ukprn); | ||
if (!isBlocked) | ||
{ | ||
return View(new UnBlockTrainingProviderEditModel() { Ukprn = model.Ukprn, ProviderName = model.ProviderName }); |
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.
If the provider is not blocked then shouldn't we navigate back to the blocked providers list and maybe show a message?
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.
👍
|
||
if (model.CanRestoreAccess) | ||
{ | ||
await _orchestrator.UnBlockProviderAsync(model.Ukprn, User.GetVacancyUser()); |
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.
I'd move the tempdata storage bit here. No reason to remember it any earlier.
TempData[TempDataKeys.UnBlockedProviderUkprnKey] = model.Ukprn;
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.
👍
[HttpGet("unblocking-acknowledgement", Name = RouteNames.UnBlockProvider_Acknowledgement_Get)] | ||
public async Task<IActionResult> ProviderUnBlockedAcknowledgement() | ||
{ | ||
var data = TempData.Peek(TempDataKeys.UnBlockedProviderUkprnKey)?.ToString(); |
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.
Do we need to peek if we are going to remove it? Maybe just read it.
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.
👍
return _messaging.SendCommandAsync(command); | ||
} | ||
|
||
public async Task<UnBlockTrainingProviderEditModel> GetBlockedOrganisationViewModel(long ukprn) |
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.
Pedantic but should UnBlock
be Unblock
?
Also GetBlockedOrganisationViewModel
is not returning what it says it is. See my comment about naming above.
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.
👍
f0d9ca9
to
9cea2a9
Compare
9cea2a9
to
7f89e66
Compare
No description provided.