Skip to content

HEEDLS-546 All delegates - register delegate - Page 3,4,5 #475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Jul 15, 2021
Merged

HEEDLS-546 All delegates - register delegate - Page 3,4,5 #475

merged 21 commits into from
Jul 15, 2021

Conversation

@ibrahimmunir14
Copy link
Contributor

Welcome Email page
localhost_44363_TrackingSystem_Delegates_Register_WelcomeEmail
localhost_44363_TrackingSystem_Delegates_Register_WelcomeEmail (1)

Password page
localhost_44363_TrackingSystem_Delegates_Register_Password

Summary page (if welcome email set)
localhost_44363_TrackingSystem_Delegates_Register_Summary

Summary page (if welcome email not set)
localhost_44363_TrackingSystem_Delegates_Register_Summary (1)

@ibrahimmunir14
HEEDLS-546 Delegate registration by centre: WelcomeEmail page GET
48f88cd
@ibrahimmunir14
HEEDLS-546 Add date validation to WelcomeEmail POST; display date fie…
2d171a7 
…lds and errors correctly on page
@ibrahimmunir14
HEEDLS-546 Read/write between ViewModel and Data on WelcomeEmail GET/…
d6e698e 
…POST
@ibrahimmunir14
HEEDLS-546 Add empty constructor to WelcomeEmail viewmodel
2239a71 
Apparently you need an empty constructor for model binding to work? Took way too long to figure that one out
@ibrahimmunir14
HEEDLS-546 Delegate registration by centre: Password page
0b236c8
@ibrahimmunir14
HEEDLS-546 Delegate registration by centre: Summary page
15555ec
@ibrahimmunir14
HEEDLS-546 Fix and tidy up welcome email date and password set
1f71475
@ibrahimmunir14
HEEDLS-546 Change back link on Summary to return to correct previous …
55da6b5 
…page; wording change; return OK on Summary submit
@ibrahimmunir14
HEEDLS-546 Make password non-required; clear password hash if welcome…
51297cc 
… email set
@ibrahimmunir14
Merge branch 'master' into HEEDLS-546-all-delegates-register-3-4-5
daf6cea
AlexJacksonDS
AlexJacksonDS reviewed Jul 8, 2021
View reviewed changes
Copy link
Contributor

@AlexJacksonDS AlexJacksonDS left a comment

Choose a reason for hiding this comment

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

A few questions/comments but overall looks good.

AlexJacksonDS
AlexJacksonDS reviewed Jul 9, 2021
View reviewed changes
Copy link
Contributor

@AlexJacksonDS AlexJacksonDS left a comment

Choose a reason for hiding this comment

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

Just one minor thing to change

stellake
stellake reviewed Jul 14, 2021
View reviewed changes
DigitalLearningSolutions.Web/Views/RegisterDelegateByCentre/WelcomeEmail.cshtml Outdated
</label>
</div>

<div class="@emailDateCss @errorCss" id="conditional-welcome-email-date">
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth putting the form error in the VC, like we do with text-inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alex suggested this too, but can't see a way to do it nicely. The error message is in the VC, but the red border styling is not. The reason being that the css for that is affected by the conditional.

...lLearningSolutions.Web.Tests/Controllers/Register/RegisterDelegateByCentreControllerTests.cs
private IJobGroupsDataService jobGroupsDataService = null!;
private IUserDataService userDataService = null!;
private IUserService userService = null!;
private ICryptoService cryptoService = null!;
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 want to add any tests about temp data being updated appropriately?

Copy link
Contributor Author

@ibrahimmunir14 ibrahimmunir14 Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've already written tests for populating DelegtateRegistrationbyCentreData from viewmodels. Is that enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone refactors this file and forgets to update TempData? I guess we could catch that in the AutomatedUI tests, when we do accessibility scans of the pages (which I'm happy for you to add as part of whichever subtask)

@ibrahimmunir14 ibrahimmunir14 requested a review from stellake July 14, 2021 15:37
stellake
stellake approved these changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@stellake stellake 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 - can we rename one of the methods, then merge this without rereview. Let's add accessibility tests as part of your second review. 🙏

@ibrahimmunir14 ibrahimmunir14 merged commit ff88697 into master Jul 15, 2021
@stellake stellake deleted the HEEDLS-546-all-delegates-register-3-4-5 branch July 15, 2021 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@stellake stellake stellake approved these changes

@AlexJacksonDS AlexJacksonDS AlexJacksonDS approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ibrahimmunir14 @stellake @AlexJacksonDS