- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
Heedls 563 course delegates basic #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small suggestions/questions.
        
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/Index.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ions.Web/Views/TrackingSystem/Delegates/CourseDelegates/_SearchableCourseDelegateCard.cshtml
          
            Show resolved
            Hide resolved
        
              
          
                ...ions.Web/Views/TrackingSystem/Delegates/CourseDelegates/_SearchableCourseDelegateCard.cshtml
          
            Show resolved
            Hide resolved
        
              
          
                ...Web/ViewModels/TrackingSystem/Delegates/CourseDelegates/SearchableCourseDelegateViewModel.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Services/CourseDelegatesService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/Services/CourseDelegatesServiceTests.cs
          
            Show resolved
            Hide resolved
        
              
          
                ...ions.Web/Views/TrackingSystem/Delegates/CourseDelegates/_SearchableCourseDelegateCard.cshtml
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/Index.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/CourseDelegates/Index.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public IEnumerable<SelectListItem> Courses { get; set; } | ||
| public bool? Active { get; set; } | ||
| 
               | 
          ||
| public IEnumerable<SearchableCourseDelegateViewModel> Delegates { get; set; } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be neater to have customisationId, Active, and Delegates live on the same nullable object, e.g. SelectedCourseDetails or something - then if there are no customisations to choose from, the data is simply null but we're forcing the CustomisationId and Active to be populated if a course does exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomisationId needs to stay on the base model as it breaks model binding otherwise
| width: 63%; | ||
| 
               | 
          ||
| @include mq($from: $xl-desktop) { | ||
| width: 80%; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these values coming from? (63% and 80%)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching the filter dropdowns for now until 580 is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO in that case with a reference to 580?
| FROM Customisations AS c | ||
| JOIN Applications AS a on a.ApplicationID = c.ApplicationID | ||
| WHERE (CentreID = @centreId OR CentreID = 0) | ||
| AND (a.CourseCategoryID = @categoryId OR @categoryId = 0)", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd still prefer the categoryId to be null with no filter rather than "0" which can be ambiguous. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admins don't have a nullable CategoryId. I think we should keep this behaviour consistent with the AdminUser records so it doesn't cause confusion unless we want to go through and change everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think in this query the "categoryId" is effectively a filter on category id, which intuitively would be:
- Null = no filter
 - Value = filter applied
 
        
          
                DigitalLearningSolutions.Data/DataServices/CourseDataService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/Services/CourseDelegatesServiceTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/Shared/_DelegatesSideNavMenu.cshtml
          
            Show resolved
            Hide resolved
        
      | width: 63%; | ||
| 
               | 
          ||
| @include mq($from: $xl-desktop) { | ||
| width: 80%; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO in that case with a reference to 580?
| FROM Customisations AS c | ||
| JOIN Applications AS a on a.ApplicationID = c.ApplicationID | ||
| WHERE (CentreID = @centreId OR CentreID = 0) | ||
| AND (a.CourseCategoryID = @categoryId OR @categoryId = 0)", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think in this query the "categoryId" is effectively a filter on category id, which intuitively would be:
- Null = no filter
 - Value = filter applied
 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)





JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-563
Description
Adds a course delegates page in the delegates section of the tracking system.
This has a dropdown to select the course that is styled the same as the filter dropdowns with no javascript, so some styling will likely have to change in the future.
Note that I've implemented the 'Archived' as an item in the details list rather than a tag as it doesn't really make sense as a tag.
Screenshots
Developer checks
I have: