HEEDLS-566 - implemented delegate progress page #643

DanBloxham-sw · 2021-09-28T12:35:15Z

JIRA link

https://softwiretech.atlassian.net/browse/HEEDLS-566

Description

Implemented a new page for showing delegate progress details.
This reused some code in pre-existing methods, so I have done a little refactoring to make things nicer.
I've also refactored the GetCourseAdminFields data service method (and the methods that call it) to no longer require a categoryId parameter that was not being used, and instead take in a bool to determine whether or not courses with the AllCentres column as 1 should be recovered.

Screenshots

Note that I forgot to put in breadcrumbs when taking the screenshots, but they are there in the code.
Desktop:

Tablet:

Mobile:

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 (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/added documentation in Swiki and/or Readme.
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 · 2021-09-28T12:44:41Z

DigitalLearningSolutions.Data/DataServices/CourseDelegatesDataService.cs

+                AND can.CentreID = @centreId AND can.CandidateId = c.CandidateId) AS AllAttempts";

        private const string AttemptsPassedQuery =
            @"(SELECT COUNT(aa.AssessAttemptID)
                FROM dbo.AssessAttempts AS aa
                INNER JOIN dbo.Candidates AS can ON can.CandidateID = aa.CandidateID
                WHERE aa.CustomisationID = cu.CustomisationID AND aa.[Status] = 1
-                AND can.CentreID = @centreId) AS AttemptsPassed";
+                AND can.CentreID = @centreId AND can.CandidateId = c.CandidateId) AS AttemptsPassed";


The addition of the check on candidate number on these query strings is not part of the requirements for this ticket, but it fixes a minor issue I saw while testing this.

Before the user reaches the new Delegate Progress page, they need to click a button on the Course Delegates expandable which also shows a pass rate. That pass rate was being calculated incorrectly as these values were being recovered incorrectly. These queries used to get all the attempts for all users on that course, rather than the individual in question. The additional check here means only the numbers for the individual are obtained, so the values are correct.

Good catch. It worries me a bit that we're relying on the Candidates table in a different query being aliased as c.
What about changing these into methods and passing the candidate id field reference in?
Then adding a mention of the candidate in the name makes calls read slightly more sensibly.
So e.g. QueryForNumAttemptsPassedByCandidate(string candidateIdFieldReference).

These are only being used on one method, and are separated into their own consts just to make the main query a bit easier to read and understand. I've pulled these definitions into the method to help restrict any of problems that might occur in future with the candidate references.

DanBloxham-sw · 2021-09-30T09:22:36Z

New screenshot of page with the inactive inset (and the breadcrumbs that were mistakenly excluded from before)

livzorn

Looks great, just a couple small questions/curiosities

livzorn · 2021-09-30T13:39:43Z

DigitalLearningSolutions.Data/DataServices/CourseAdminFieldsDataService.cs

+        public CourseAdminFieldsResult GetCourseAdminFields(
+            int customisationId,
+            int centreId,
+            bool allowAllCentreCourses = false


I like this change - just a heads up that I removed categoryId from this method altogether in another MR, so whoever's gets merged first will have to look out for merge conflicts. (It seemed like it was repeating my service filter check but I wasn't thinking about this method being used in a delegate context)

DigitalLearningSolutions.Data/DataServices/CourseDataService.cs

...Solutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/_DelegateProgressSummary.cshtml

SteveJacksonSoft

Got about halfway through before my flight. Some changes worth making.

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

DigitalLearningSolutions.Data/DataServices/CourseDataService.cs

SteveJacksonSoft · 2021-10-01T19:01:39Z

DigitalLearningSolutions.Data/DataServices/CourseDelegatesDataService.cs

+                AND can.CentreID = @centreId AND can.CandidateId = c.CandidateId) AS AllAttempts";

        private const string AttemptsPassedQuery =
            @"(SELECT COUNT(aa.AssessAttemptID)
                FROM dbo.AssessAttempts AS aa
                INNER JOIN dbo.Candidates AS can ON can.CandidateID = aa.CandidateID
                WHERE aa.CustomisationID = cu.CustomisationID AND aa.[Status] = 1
-                AND can.CentreID = @centreId) AS AttemptsPassed";
+                AND can.CentreID = @centreId AND can.CandidateId = c.CandidateId) AS AttemptsPassed";


Good catch. It worries me a bit that we're relying on the Candidates table in a different query being aliased as c.
What about changing these into methods and passing the candidate id field reference in?
Then adding a mention of the candidate in the name makes calls read slightly more sensibly.
So e.g. QueryForNumAttemptsPassedByCandidate(string candidateIdFieldReference).

DigitalLearningSolutions.Data/Models/Courses/DelegateCourseDetails.cs

DigitalLearningSolutions.Data.Tests/Services/CourseServiceTests.cs

SteveJacksonSoft

Looks good, just a couple of things that could be improved on the view.

...LearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/DelegateProgress.cshtml

SteveJacksonSoft

Nice, merge away.

HEEDLS-566 - implemented delegate progress page

71acbd6

DanBloxham-sw commented Sep 28, 2021

View reviewed changes

DanBloxham-sw added 4 commits September 28, 2021 14:06

HEEDLS-566 - fix query indentation and replace removed condition

c11df7d

HEEDLS-566 - apply word break to email

4af4d1e

HEEDLS-566 - add breadcrumbs to page

eea8970

HEEDLS-566 - added inactive course inset to page

dce840b

livzorn reviewed Sep 30, 2021

View reviewed changes

HEEDLS-566 - remove unnecessary schema specification

5048892

livzorn approved these changes Oct 1, 2021

View reviewed changes

SteveJacksonSoft changed the base branch from master to HEEDLS-351-recaptcha October 1, 2021 17:49

SteveJacksonSoft changed the base branch from HEEDLS-351-recaptcha to master October 1, 2021 17:49

SteveJacksonSoft suggested changes Oct 1, 2021

View reviewed changes

DanBloxham-sw added 3 commits October 4, 2021 10:23

Merge branch master in HEEDLS-566

ffc0699

HEEDLS-566 - minor tech lead markups

ab2d116

HEEDLS-566 - make user permissions check static

1947a8c

SteveJacksonSoft suggested changes Oct 5, 2021

View reviewed changes

...LearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/DelegateProgress.cshtml Outdated Show resolved Hide resolved

...LearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/DelegateProgress.cshtml Show resolved Hide resolved

HEEDLS-566 - switch to using AutoNavMenu

9469ee2

SteveJacksonSoft approved these changes Oct 6, 2021

View reviewed changes

DanBloxham-sw merged commit ca09e40 into master Oct 6, 2021

DanBloxham-sw deleted the HEEDLS-566-DelegateCourseProgressPage branch October 6, 2021 13:43

HEEDLS-566 - implemented delegate progress page #643

HEEDLS-566 - implemented delegate progress page #643

Uh oh!

Conversation

DanBloxham-sw commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA link

Description

Screenshots

Developer checks

Uh oh!

DanBloxham-sw Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

SteveJacksonSoft Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

DanBloxham-sw Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

DanBloxham-sw commented Sep 30, 2021

Uh oh!

livzorn left a comment

Choose a reason for hiding this comment

Uh oh!

livzorn Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteveJacksonSoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteveJacksonSoft Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SteveJacksonSoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SteveJacksonSoft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanBloxham-sw commented Sep 28, 2021 •

edited

Loading