Skip to content

HEEDLS-374 Centre Configuration - Edit centre details #430

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 18 commits into from
Jul 15, 2021
Merged

HEEDLS-374 Centre Configuration - Edit centre details #430

merged 18 commits into from
Jul 15, 2021

Conversation

@davidm-m
Copy link
Contributor

@davidm-m davidm-m commented Jun 18, 2021 •
edited
Loading

Adds the edit centre details page to the Tracking System. Includes a new image resize method that ensures no side length of an image exceeds a certain pixel value.
image

@davidm-m
Copy link
Contributor Author

ImageResizeService currently doesn't have CRLF line endings. I've left it for ease of reviewing but please remind me to fix that before merging 🙏

davidm-m
davidm-m commented Jun 18, 2021
View reviewed changes
DigitalLearningSolutions.Data/Services/ImageResizeService.cs Outdated
Comment on lines 96 to 97
var ratioX = Math.Min((float)maxSideLengthPx / (float)image.Width, 1);
var ratioY = Math.Min((float)maxSideLengthPx / (float)image.Height, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that we don't accidentally enlarge a small logo or signature - I thought that would be the expected behaviour, happy to be corrected though

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me! 👍

DanBloxham-sw
DanBloxham-sw reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@DanBloxham-sw DanBloxham-sw left a comment

Choose a reason for hiding this comment

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

Few minor questions. Also looks like there is a small merge conflict around the new Edit button and Preview Certificate button.

davidm-m added 2 commits June 30, 2021 16:25
@davidm-m
HEEDLS-374 Added and updated tests and made NotifyEmail optional
bf41651
@davidm-m
Merge branch 'master' into HEEDLS-374-edit-centre
152e2e6 
# Conflicts:
#	DigitalLearningSolutions.Data.Tests/DataServices/CentresDataServiceTests.cs
#	DigitalLearningSolutions.Web/Views/TrackingSystem/CentreConfiguration/_CentreConfigurationCentreDetails.cshtml
DanBloxham-sw
DanBloxham-sw approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@DanBloxham-sw DanBloxham-sw 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 to me. Looks like the most recent build/test for github was cancelled, might be worth rerunning this.
Also here is a reminder to sort the line endings on that file you mentioned in another comment once it gets through Tech Lead review.

stellake
stellake reviewed Jul 1, 2021
View reviewed changes
DigitalLearningSolutions.Data/Services/ImageResizeService.cs Outdated
Comment on lines 96 to 97
var ratioX = Math.Min((float)maxSideLengthPx / (float)image.Width, 1);
var ratioY = Math.Min((float)maxSideLengthPx / (float)image.Height, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me! 👍

@stellake
Copy link
Contributor

stellake commented Jul 1, 2021

Forgot to mention but I found the preview image & the Incognito mode combo very entertaining 😄

@davidm-m
Copy link
Contributor Author

davidm-m commented Jul 1, 2021

Forgot to mention but I found the preview image & the Incognito mode combo very entertaining 😄

It's not incognito! I just have Firefox in dark mode 😅

stellake
stellake reviewed Jul 15, 2021
View reviewed changes
...olutions.Web/Controllers/TrackingSystem/CentreConfiguration/RegistrationPromptsController.cs
private IActionResult AddRegistrationPromptConfigureAnswersPostNext(RegistrationPromptAnswersViewModel model)
{
IgnoreAddNewAnswerValidation();
ModelState.ClearAllErrors();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not your change but is the if-statement below ever true when we clear the errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not. Both this method and EditRegistrationPromptPostSave previously called IgnoreAddNewAnswerValidation before checking ModelState.IsValid which does exactly the same thing as ModelState.ClearAllErrors. I think they might have been holdovers that were never removed, so I'll remove them now. But if they are necessary for some bizarre reason, it will be easy enough to add them back.

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 - just a comment about other bit of the system that we should check 👍

@DanBloxham-sw DanBloxham-sw merged commit 152ca9c into master Jul 15, 2021
@DanBloxham-sw DanBloxham-sw deleted the HEEDLS-374-edit-centre branch July 15, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@DanBloxham-sw DanBloxham-sw DanBloxham-sw approved these changes

@stellake stellake stellake 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

@davidm-m @stellake @DanBloxham-sw