HEEDLS-555 View delegate page - course section #498

ibrahimmunir14 · 2021-07-21T13:10:59Z

Supervisor: YES; Completed: NO; Assessed: YES; Attempts: NO; Custom fields: NO

Supervisor: NO; Completed: YES; Assessed: YES; Attempts: YES; Custom fields: NO

Supervisor: NO; Completed: NO; Assessed: NO; Attempts: NO; Custom fields: YES

Tablet

Mobile

…fo data service method; write test

…ate controller; update tests

…ds in View Delegate page

…wers list to DelegateCourseInfo VM

…e to service method

…ler; write data service method; write tests

…oCard

DanBloxham-sw

Few minor things, might require some refactoring to use CustomPromptWithAnswer class.

DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Delegates/DelegateCourseInfoViewModel.cs

DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/Index.cshtml

...ningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/_DelegateCourseInfoCard.cshtml

DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Delegates/DelegateCourseInfoViewModel.cs

add service method; write test; update view/vm/controller

simplify nullable date assignments; add empty actions dd to fix row separators; fix swapped field values; remove unneeded role=alert

…CourseInfo

DanBloxham-sw

Looks good. Minor comment about renaming a display field, but no need for reapproval from me.

I think the empty action on all the fields should fix the line issue, but it would be nice to have an updated screen shot as well.

...ningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/_DelegateCourseInfoCard.cshtml

…or ViewDelegate accessibility test to avoid timeout

Fix problem of lonely % sign when value is null, without needing an extra check in the view

...ningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/_DelegateCourseInfoCard.cshtml

DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/Index.cshtml

DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Delegates/ViewDelegateViewModel.cs

DigitalLearningSolutions.Data/DataServices/CourseDataService.cs

stellake · 2021-07-26T08:37:46Z

DigitalLearningSolutions.Data/DataServices/CustomPromptsDataService.cs

                    INNER JOIN dbo.Applications AS ap ON ap.ApplicationID = cu.ApplicationID
-                    WHERE (ap.CourseCategoryID = @categoryId OR @categoryId = 0) 
-                        AND cu.CentreID = @centreId
-                        AND ap.ArchivedDate IS NULL


We should never need to pull archived courses to my knowledge - any reason for this change?

We decided to move these filters into the service.
CategoryId filter needed to be optional, so we added service methods to make it cleaner.
Then I wasn't sure whether or not archived courses were included in delegate courses, so pulled that filter out too for consistency. If it's more flexible than needed I can put the archived date filter back into the sql?

I think we should keep the Archived filter in the SQL for now, we can always bring it back if we need to in the future, but it seems unlikely that we would want to show archived courses.

stellake · 2021-07-26T09:04:07Z

DigitalLearningSolutions.Data/Services/CustomPromptsService.cs

 namespace DigitalLearningSolutions.Data.Services
 {
    using System.Collections.Generic;
    using System.Linq;


This is getting quite long - I wonder if we could refactor this into two separate services, one for registration prompts and the other for admin fields.. Olivia is working on this too atm, so she could potentially do the refactor later.

I agree with this, it's at a similar length to the UserDataService I've just split. @livzorn FYI, this split might come your way at the end of the admin fields work.

@AlexJacksonDS ah yeah thanks, I'll keep that in mind!

stellake · 2021-07-26T09:09:52Z

DigitalLearningSolutions.Data/DataServices/CourseDataService.cs

+                @"SELECT COUNT(aa.Status) AS TotalAttempts,
+                        COUNT(CASE WHEN aa.Status=1 THEN 1 END) AS AttemptsPassed


I wonder if it makes sense pulling these two numbers together with other course data?

I thought it would make the query too complicated. Also these are not needed in many cases any where IsAssessed=0.

I think there is a bit more refactoring needed in that case (also see my comment on the controller). If these aren't needed because IsAssessed=0, why are we still always calling it?

… combine in code; add tests

AlexJacksonDS

A few things that I think need changing.

DigitalLearningSolutions.Data.Tests/DataServices/CourseDataServiceTests.cs

AlexJacksonDS · 2021-07-28T10:39:38Z

DigitalLearningSolutions.Data/DataServices/CourseDataService.cs

+                @"SELECT COUNT(aa.Status) AS TotalAttempts,
+                        COUNT(CASE WHEN aa.Status=1 THEN 1 END) AS AttemptsPassed


I think there is a bit more refactoring needed in that case (also see my comment on the controller). If these aren't needed because IsAssessed=0, why are we still always calling it?

AlexJacksonDS · 2021-07-28T10:43:33Z

DigitalLearningSolutions.Data/DataServices/CustomPromptsDataService.cs

                    INNER JOIN dbo.Applications AS ap ON ap.ApplicationID = cu.ApplicationID
-                    WHERE (ap.CourseCategoryID = @categoryId OR @categoryId = 0) 
-                        AND cu.CentreID = @centreId
-                        AND ap.ArchivedDate IS NULL


I think we should keep the Archived filter in the SQL for now, we can always bring it back if we need to in the future, but it seems unlikely that we would want to show archived courses.

DigitalLearningSolutions.Data/Services/CustomPromptsService.cs

DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/ViewDelegateController.cs

DigitalLearningSolutions.Web/Styles/trackingSystem/viewDelegate.scss

Also don't fetch attempt stats if course not assessed

…ved parameter

AlexJacksonDS

Some remaining ArchivedDate to remove still.
I don't think an IEnumerable<(DelegateCourseInfo info, List prompts, (int, int) stats)> is a good idea really. Replace this with an IEnumerable of a new class
New CourseService method needs unit tests, unless I've missed them?

DigitalLearningSolutions.Data/Services/CourseService.cs

DigitalLearningSolutions.Data.Tests/TestHelpers/CustomPromptsTestHelper.cs

DigitalLearningSolutions.Data/DataServices/CustomPromptsDataService.cs

DigitalLearningSolutions.Data/Services/CourseService.cs

…t stats, custom fields

AlexJacksonDS

Looks OK to me now 👍

ibrahimmunir14 added 10 commits July 20, 2021 18:08

HEEDLS-555 Create DelegateCourseInfo model; write GetDelegateCourseIn…

0f1711e

…fo data service method; write test

HEEDLS-555 Create DelegateCourseInfoViewModel; write tests

89fcb17

HEEDLS-555 Add CourseInfoViewModel list to ViewDelegateViewModel; upd…

7800fc2

…ate controller; update tests

HEEDLS-555 Add CustomisationId to DelegateCourseInfo

649b221

HEEDLS-555 Create _DelegateCourseInfoCard partial; display course car…

4b230ed

…ds in View Delegate page

HEEDLS-555 Add custom Answer1-3 to DelegateCourseInfo; add custom ans…

f0879e8

…wers list to DelegateCourseInfo VM

HEEDLS-555 Move CategoryID and ArchivedDate filtering from dataservic…

1156114

…e to service method

HEEDLS-555 Add custom prompts to DelegateCourseInfo

69baeb7

HEEDLS-555 Add attempts stats to DelegateCourseInfo VM, view, control…

49c6a88

…ler; write data service method; write tests

HEEDLS-555 Add Edit and Set completed date links to DelegateCourseInf…

1c24175

…oCard

DanBloxham-sw reviewed Jul 22, 2021

View reviewed changes

ibrahimmunir14 added 3 commits July 22, 2021 13:53

HEEDLS-555 Use CustomPromptWithAnswer instead of CustomPrompt

06b21e6

add service method; write test; update view/vm/controller

HEEDLS-555 Review markups

feb5c4b

simplify nullable date assignments; add empty actions dd to fix row separators; fix swapped field values; remove unneeded role=alert

HEEDLS-555 Use pass rate percentage instead of pass ratio in Delegate…

201f7c6

…CourseInfo

DanBloxham-sw approved these changes Jul 22, 2021

View reviewed changes

...ningSolutions.Web/Views/TrackingSystem/Delegates/ViewDelegate/_DelegateCourseInfoCard.cshtml Outdated Show resolved Hide resolved

ibrahimmunir14 added 3 commits July 22, 2021 17:14

HEEDLS-555 Update pass rate field name; use alternative delegate id f…

eff0f8e

…or ViewDelegate accessibility test to avoid timeout

HEEDLS-555 Move pass rate double to percentage string conversion into VM

7f1b13e

Fix problem of lonely % sign when value is null, without needing an extra check in the view

HEEDLS-555 Fix misspelling of enrolment

63c681a

stellake reviewed Jul 26, 2021

View reviewed changes

ibrahimmunir14 added 3 commits July 26, 2021 12:07

HEEDLS-555 Add CourseName derived field on DelegateCourseInfoVM

9cdeff7

HEEDLS-555 Minor refactoring and renaming

46f103f

HEEDLS-555 Select Supervisor Forename and Surname separately from db;…

7365ac3

… combine in code; add tests

ibrahimmunir14 requested a review from stellake July 26, 2021 13:11

Merge branch 'master' into HEEDLS-555-view-delegate-page-course-section

a1d4291

AlexJacksonDS suggested changes Jul 28, 2021

View reviewed changes

ibrahimmunir14 added 4 commits July 28, 2021 13:28

HEEDLS-555 Move delegate course info fetching logic into service method

6534359

Also don't fetch attempt stats if course not assessed

HEEDLS-555 Put ArchivedDate check back into SQL query; remove isArchi…

72bafe5

…ved parameter

HEEDLS-555 View delegate summary style change; add comment

63bf216

Merge branch 'master' into HEEDLS-555-view-delegate-page-course-section

f3f9b87

ibrahimmunir14 requested a review from AlexJacksonDS July 28, 2021 13:31

AlexJacksonDS suggested changes Jul 28, 2021

View reviewed changes

ibrahimmunir14 added 5 commits July 28, 2021 16:01

HEEDLS-555 Remove remaining ArchivedDate

5ef43f9

HEEDLS-555 Create DelegateCourseDetails model for course info, attemp…

55fc34c

…t stats, custom fields

HEEDLS-555 Fix DelegateCourseInfo VM tests

807aa16

HEEDLS-555 Add ArchivedDate IS NULL check to GetDelegateCoursesInfo

5d682dd

HEEDLS-555 Add CourseService tests for GetAllCoursesForDelegate

284ceaf

ibrahimmunir14 requested a review from AlexJacksonDS July 28, 2021 17:09

AlexJacksonDS approved these changes Jul 29, 2021

View reviewed changes

ibrahimmunir14 merged commit 661d517 into master Jul 29, 2021

stellake deleted the HEEDLS-555-view-delegate-page-course-section branch August 11, 2021 20:06

		@"SELECT COUNT(aa.Status) AS TotalAttempts,
		COUNT(CASE WHEN aa.Status=1 THEN 1 END) AS AttemptsPassed

HEEDLS-555 View delegate page - course section #498

HEEDLS-555 View delegate page - course section #498

Uh oh!

Conversation

ibrahimmunir14 commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Supervisor: YES; Completed: NO; Assessed: YES; Attempts: NO; Custom fields: NO

Supervisor: NO; Completed: YES; Assessed: YES; Attempts: YES; Custom fields: NO

Supervisor: NO; Completed: NO; Assessed: NO; Attempts: NO; Custom fields: YES

Tablet

Mobile

Uh oh!

DanBloxham-sw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanBloxham-sw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJacksonDS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexJacksonDS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexJacksonDS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ibrahimmunir14 commented Jul 21, 2021 •

edited

Loading