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

ER-1377_show link to restore access for blocked provider. #810

Merged
merged 10 commits into from
Sep 23, 2019

Conversation

shomavg
Copy link
Contributor

@shomavg shomavg commented Sep 6, 2019

No description provided.

Copy link
Contributor

@chrisjensenuk chrisjensenuk left a 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";
Copy link
Contributor

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

Copy link
Contributor Author

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 });
Copy link
Contributor

@chrisjensenuk chrisjensenuk Sep 11, 2019

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

Copy link
Contributor Author

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 });
Copy link
Contributor

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?

Copy link
Contributor Author

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());
Copy link
Contributor

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;

Copy link
Contributor Author

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();
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@shomavg shomavg merged commit e1f9d16 into master Sep 23, 2019
@shomavg shomavg deleted the ER-1377_unblocking-provider branch September 23, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants