Skip to content

Conversation

@livzorn
Copy link
Contributor

@livzorn livzorn commented Jan 18, 2022

JIRA link

HEEDLS-551

Description

Multi page form for adding a new course.

I ended up refactoring the validation on EditCourseDetails, moving the methods from the ManageCourseController to a helper file so they could be used in the CourseSetupController.

I also created an EditSectionContentHelper file to commonise the code around selecting/deselecting with the CourseContentController.

Screenshots

localhost_5001_TrackingSystem_CourseSetup

localhost_5001_TrackingSystem_CourseSetup_AddCourse_SelectCourse

localhost_5001_TrackingSystem_CourseSetup_AddCourse_SetCourseDetails

localhost_5001_TrackingSystem_CourseSetup_AddCourse_SetCourseOptions

localhost_5001_TrackingSystem_CourseSetup_AddCourse_SetCourseContent

localhost_5001_TrackingSystem_CourseSetup_AddCourse_SetSectionContent_nextSectionIndex=1

localhost_5001_TrackingSystem_CourseSetup_AddCourse_Summary

localhost_5001_TrackingSystem_CourseSetup_AddCourse_Confirmation


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:
  • 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.

Comment on lines -484 to -486
public bool DoesCourseExistAtCentre(int customisationId, int centreId, int? categoryId)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed this method was unused so I removed it. It's from another ticket I worked on when I made a new version of it and forgot to remove the old.

Comment on lines -514 to -523
[Test]
public void DoesCourseExistAtCentre_returns_true_if_course_exists()
{
// When
var result = courseDataService.DoesCourseExistAtCentre(100, 101, null);

// Then
result.Should().BeTrue();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These removed tests were just for the unused method I removed

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.

I think I've covered most of the stuff here, but this is quite a large MR so I may find more stuff when it comes to re-review.

@livzorn livzorn force-pushed the HEEDLS-551-add-new-course branch from 1142983 to a96412d Compare January 19, 2022 12:21
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.

Marked some of the things I think might need moving in the controller

@livzorn livzorn force-pushed the HEEDLS-551-add-new-course branch from a96412d to f523297 Compare January 21, 2022 09:14
@livzorn livzorn force-pushed the HEEDLS-551-add-new-course branch from 60188a9 to 58d9238 Compare January 25, 2022 17:29
Copy link
Contributor

@wilkinsm wilkinsm left a comment

Choose a reason for hiding this comment

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

Nice one Olivia, loads of work gone in here!
I've left some suggestions and changes to look at.

Copy link
Contributor

@wilkinsm wilkinsm left a comment

Choose a reason for hiding this comment

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

Nice one Olivia, looks good to merge.

@livzorn livzorn merged commit b73841d into master Jan 27, 2022
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.

4 participants