HEEDLS-594 Deactivate Admin Account - test fix

[showkath10]

JIRA link

https://softwiretech.atlassian.net/browse/HEEDLS-594

Description

Additional left margin to the user text is removed
Added error summary block

Screenshots

---

Developer checks

(Leave tasks unticked if they haven't been appropriate for your ticket.)

I have:

• Run the formatter and made sure there are no IDE errors.
• Written tests for the changes (accessibility tests, unit tests for controller, data services, services, view models, etc)
• Manually tested my work with and without JavaScript. Full manual testing guidelines can be found here: https://softwiretech.atlassian.net/wiki/spaces/HEE/pages/6703648740/Testing
• Updated/added documentation in Swiki and/or Readme. Links (if any) are below:
  • doc_1_here
• Updated my Jira ticket with information about other parts of the system that were touched as part of the MR and have to sanity tested to ensure nothing’s broken.
• Scanned over my own MR to ensure everything is as expected.

[wilkinsm]

Extra space here is not required.

[AlexJacksonDS]

I had a look through the history of this ticket to understand why the strange spacing raised as a bug occurred, and I found that several comments from the original MR are still unresolved here (plus some extra thoughts that I think are incorrect) having got lost in the Closing/New MR change. I can't comment them against the code since it isn't changed here, so I'm just going to raise them all in this comment with screenshots of the relevant lines from VS.

1. Martin's comment above is not yet resolved

2. DeactivateAdmin.cshtml: We have an extra div nhsuk-form-group that is also contained in the view component. This View component also has populate-with-current-value that is never used. I also think we should perhaps replace this with SingleCheckboxViewComponent, although that does behave slightly differently.
   

3. ConfirmCheckboxViewComponent: I think we should perhaps get rid of this in favour of SingleCheckboxViewComponent, but their behaviour for errors is slightly different. Single checkbox only shows one error, but I'm not sure why ConfirmCheckbox needs to show multiple errors?

4. DeactivateAdminVewModel.cs: FullName should not be nullable, the AdminUser property we are populating into it isn't. UserId isn't used anywhere outside the constructor, remove it.
   

5. Steve's original comment about spacing has been reverted by this MR (It was done incorrectly before). We want vertical spacing, but no horizontal spacing, hence the bug.

6. AdminUserDataServiceTests.cs: Relating to this comment, we're still using hard coded 7 (and more of them now).

7. AdministratorController.cs: There is an unused call to get the admin user by ID here. Remove it.
   

8. AdministratorControllerTests.cs: Relating to this comment for DeactivateAdminUser_deactivates_admin_user_with_confirmation, we're not using NBuilder or just creating a new DeactivateAdminViewModel using its parameterless contructor. We also have these configured calls to GetAdminUserById which aren't needed since that statement is unused (see 7.).
   

I believe all of your other comments from the previous MRs have been resolved @SteveJacksonSoft @davidm-m

[AlexJacksonDS]

See above comment

[SteveJacksonSoft]

For the record...
Showkath and I did have a discussion about ConfirmCheckboxVC. It came along before SingleCheckboxVC, but I prefer the latter because it's more generic. So I agree to getting rid of CCVC.
It feels like unnecessary logic to prune your list of error messages down to a single one in the SingleCheckboxVC, it's simpler for it just to show what it's given, but it's not a big deal because as Alex says, why would you have > 1 validation message for a binary input.

[AlexJacksonDS]

Just a couple of things left to resolve now.