Heedls 443 edit learning pathway defaults

[livzorn]

JIRA link

HEEDLS-443

Description

Added a new page at TrackingSystem/CourseSetup/{customisationId}/Manage/LearningPathwayDefaults with a form to edit the learning pathway defaults of a course. The controller contains some custom validations and error messages, checking whether the inputs for CompleteWithinMonths and ValidityMonths are numbers and within the range of 0 to 48 inclusive. If the input is null those values are saved as 0 (those columns are non-nullable) so that the user can leave them blank if they do not want to configure values.

Screenshots

---

Developer checks

I have:

• Run the formatter and made sure there are no IDE errors.
• Written tests for the changes (controller, data services, services, view models etc) and 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 my Jira ticket with information about other parts of the system that were touched as part of the MR and have to be sanity tested to ensure nothing’s broken.
• Scanned over my own MR to ensure everything is as expected.

[DanBloxham-sw]

A few points, mostly minor.

[DanBloxham-sw]

We should have a checkboxes view component that takes in a list of view models that could be used in here unless there's a good reason I am missing. See the EditAdminFields in the Tracking System for an example.

Looking at the screenshots, we could also do with a little more space between the checkboxes and the text input above.

[livzorn]

I think that component has a non nullable label for the group of checkboxes so it's meant more for checkboxes that are specifically related to each other, whereas I feel like these are independent.

I've added more space between them, how does this look?

[DanBloxham-sw]

I think the spacing looks good here.

I see what you mean about the checkboxes view component as well. For reference, I don't believe that view components can have nullable/optional parameters, but you can put an empty string in (though in this case it would still create some HTML without any content). David is implementing a single-checkbox view component in 512, so if that ends up getting merged before this, it might be worth including it, but for now its probably fine as is.

[livzorn]

Oh right, of course you could just leave the label blank :) I will use David's checkbox component once it's merged!

[DanBloxham-sw]

Minor things, mostly looks like a namespace got messed up.

[DanBloxham-sw]

Looking good.

[SteveJacksonSoft]

It would be nice to make clear that these bounds are inclusive (i.e. 35 is within the range 4-35).

I think adding inclusive into either the attribute name or the parameter names is the way to go.

[livzorn]

Good idea, renamed to WholeNumberWithinInclusiveRangeAttribute

[SteveJacksonSoft]

You could reduce these to a single test using [TestCaseSource] like in LinkedFieldHelperTests.cs.

It's nice to have the names of the tests to tell us what each case is for. You could keep that by passing a failure message in with the test data and giving it to NUnit for when the test fails. I believe FluentAssertions lets you do that quite easily. This article should send you in the right direction, though may not have everything you need.

[livzorn]

I've used [TestCaseSource] to refactor the tests and I'm passing in a custom test name with each one :)

[AlexJacksonDS]

A few things that need tidying up, mainly around the new view components.