HEEDLS-659 - implemented action plan resource on current page sans fe… #780

DanBloxham-sw · 2021-11-30T13:49:37Z

…ature flag

JIRA link

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

Description

Implemented the retrieval of incomplete action plan items for display the Current page in the learning portal. Did some minor refactoring of the existing page.

Screenshots

I've shown a few resources below (all have come from the API so presumably match how we expect the live resources to be).

One that has not been expanded
Expanded, but with empty description
Expanded with description
Expanded with overdue due date

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 (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:
- doc_1_here
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.

…ature flag

DanBloxham-sw · 2021-11-30T13:57:48Z

...arningSolutions.Web/Views/LearningPortal/Current/CurrentCourseCard/_CurrentCourseCard.cshtml

 @model CurrentCourseViewModel

-<div class="searchable-element nhsuk-panel @Model.DateStyle()" id="@Model.Id-card">
+<div class="searchable-element nhsuk-panel @Model.DateStyle()" id="@Model.Id-course-card">


I noticed that the current course card and self assessment card used the same Id template ("{Id}-card") which could conflict on the off chance the id of a self assessment matched the id of a course (which could be possible given they are at different tables). So I took the liberty to change these here.

DanBloxham-sw · 2021-11-30T13:59:47Z

DigitalLearningSolutions.Web/Views/LearningPortal/Shared/_TagRow.cshtml

    }
  }
+  @if (Model is LearningResourceCardViewModel resource) {
+    <div class="tag nhsuk-u-font-size-14" aria-describedby="@Model.Id-name">


I just gave these tags the same styling as the tags in the other Current cards for courses and self assessments. So these are consistent with the page, but not consistent with tags we use elsewhere (e.g. tracking system filters).

Have a look at the screenshots and see if they look too much like buttons. Otherwise I can probably refactor the tags to look more like the tags we use elsewhere.

I do think the tags look confusingly like buttons a bit too much, especially because they are the same color as the Mark as complete and Edit buttons. I don't know if "Image" and "WebLink" are test values or values likely to be displayed in the tags, but if they are, it seems likely users would click on them thinking that will open the image or link. I think it would be worth making they look like the tags we use elsewhere.

I've updated the tags on the learning resource cards (and added new tags for each type). I've not updated the old course and self assessment tags yet. I will ask Kevin a question in the slack thread about the new tags to see what he thinks of them. Images below:

I think that looks a lot better! And I support changing them on the other cards for consistency as long as Kevin okays that

DanBloxham-sw · 2021-11-30T16:05:54Z

Still need to implement the UseSignposting feature management flag now that it is merged.

…items

DanBloxham-sw · 2021-12-01T11:38:17Z

UseSignposting feature management flag is now being used.

DanBloxham-sw · 2021-12-01T11:39:02Z

DigitalLearningSolutions.Data/Helpers/ConfigHelper.cs

        private const string LearningHubOpenApiKey = "LearningHubOpenAPIKey";
        private const string LearningHubOpenApiBaseUrl = "LearningHubOpenAPIBaseUrl";
-        private const string UseSignposting = "FeatureManagement:UseSignposting";
+        public const string UseSignposting = "FeatureManagement:UseSignposting";


Found out why it's good to have this as public - useful for unit tests.

Fair enough

livzorn

Looking good, just a few questions and styling suggestions

DigitalLearningSolutions.Data.Tests/DataServices/LearningLogItemsDataServiceTests.cs

livzorn · 2021-12-01T14:45:04Z

DigitalLearningSolutions.Data/DataServices/LearningLogItemsDataService.cs

+                        SeqInt,
+                        LastAccessedDate,
+                        LinkedCompetencyLearningResourceID,
+						clr.LHResourceReferenceID AS LearningHubResourceReferenceID


slightly weird indentation here?

DigitalLearningSolutions.Web/Views/LearningPortal/Current/_CurrentLearningResourceCard.cshtml

livzorn · 2021-12-01T15:10:44Z

DigitalLearningSolutions.Web/Views/LearningPortal/Shared/_TagRow.cshtml

    }
  }
+  @if (Model is LearningResourceCardViewModel resource) {
+    <div class="tag nhsuk-u-font-size-14" aria-describedby="@Model.Id-name">


I do think the tags look confusingly like buttons a bit too much, especially because they are the same color as the Mark as complete and Edit buttons. I don't know if "Image" and "WebLink" are test values or values likely to be displayed in the tags, but if they are, it seems likely users would click on them thinking that will open the image or link. I think it would be worth making they look like the tags we use elsewhere.

DigitalLearningSolutions.Web/Views/LearningPortal/Current/_CurrentLearningResourceCard.cshtml

livzorn

Looking good, just left one more nickpicky styling suggestion which you can take or leave, and seems like the one weird indentation is still there on the sql is still there, but no need for rereview. I'll move this along so we can get more eyes on it sooner :)

AlexJacksonDS

A few comments around formatting.

DigitalLearningSolutions.Data/DataServices/LearningLogItemsDataService.cs

AlexJacksonDS · 2021-12-02T15:43:20Z

DigitalLearningSolutions.Data/Helpers/ConfigHelper.cs

        private const string LearningHubOpenApiKey = "LearningHubOpenAPIKey";
        private const string LearningHubOpenApiBaseUrl = "LearningHubOpenAPIBaseUrl";
-        private const string UseSignposting = "FeatureManagement:UseSignposting";
+        public const string UseSignposting = "FeatureManagement:UseSignposting";


Fair enough

DigitalLearningSolutions.Web/Views/LearningPortal/Current/AllCurrentItems.cshtml

DigitalLearningSolutions.Web/Views/LearningPortal/Current/_CurrentLearningResourceCard.cshtml

DigitalLearningSolutions.Web/Views/LearningPortal/SelfAssessments/_SelfAssessmentCard.cshtml

DigitalLearningSolutions.Data/Services/ActionPlanService.cs

DanBloxham-sw · 2021-12-03T10:31:10Z

DigitalLearningSolutions.Data/Services/ActionPlanService.cs

+
+        Task<IEnumerable<ActionPlanItem>> GetIncompleteActionPlanItems(int delegateId);
+
+        Task<string?> AccessLearningResource(int learningLogItemId, int delegateId);


Couldn't really think of a good name for this. Thoughts on this? Might be worth a refactor?

I like how short and sweet it is but it does seem a bit ambiguous. The only other thing I can think of is GetLearningResourceLinkAndUpdateLastAccessedDate, which is a mouthful but would be clearer

AlexJacksonDS

I think this looks OK now, layout of the buttons on the cards is still a bit strange, but that is how they were before. At least we don't have tags styled the same way as buttons any more.

HEEDLS-659 - implemented action plan resource on current page sans fe…

8fadd66

…ature flag

DanBloxham-sw commented Nov 30, 2021

View reviewed changes

HEEDLS-659 - fix mobile button spacing, add todo comment

1e81f89

DanBloxham-sw added 2 commits December 1, 2021 10:49

Merge branch 'master' into HEEDLS-659-ShowActionPlanInCurrent

a8d2546

HEEDLS-659 - implement signposting feature management to action plan …

e8f8a7c

…items

DanBloxham-sw commented Dec 1, 2021

View reviewed changes

livzorn suggested changes Dec 1, 2021

View reviewed changes

DanBloxham-sw added 2 commits December 2, 2021 12:05

Merge branch master into HEEDLS-659

be741ae

HEEDLS-659 - review markups and new tags

9ce6eae

livzorn approved these changes Dec 2, 2021

View reviewed changes

AlexJacksonDS suggested changes Dec 2, 2021

View reviewed changes

HEEDLS-659 - switch tags, update LastAccess, review markups

b2c5fa1

DanBloxham-sw commented Dec 3, 2021

View reviewed changes

HEEDLS-659 - rename AccessLearningResource to be more descriptive

0a253b8

AlexJacksonDS approved these changes Dec 6, 2021

View reviewed changes

DanBloxham-sw merged commit 90e43ab into master Dec 6, 2021

DanBloxham-sw deleted the HEEDLS-659-ShowActionPlanInCurrent branch December 6, 2021 09:55


		Task<IEnumerable<ActionPlanItem>> GetIncompleteActionPlanItems(int delegateId);

		Task<string?> AccessLearningResource(int learningLogItemId, int delegateId);

HEEDLS-659 - implemented action plan resource on current page sans fe… #780

HEEDLS-659 - implemented action plan resource on current page sans fe… #780

Uh oh!

Conversation

DanBloxham-sw commented Nov 30, 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!

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!

DanBloxham-sw commented Nov 30, 2021

Uh oh!

DanBloxham-sw commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

livzorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

livzorn left a comment

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!

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!

AlexJacksonDS 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 Nov 30, 2021 •

edited

Loading