- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
Heedls 711 filter results outside range #855
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
Heedls 711 filter results outside range #855
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.
Looks just about how I was expecting, though I've left a few comments to tackle.
        
          
                DigitalLearningSolutions.Data/Services/RecommendedLearningService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Services/RecommendedLearningService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Services/RecommendedLearningService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Services/RecommendedLearningService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/Services/RecommendedLearningServiceTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/Services/RecommendedLearningServiceTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/Services/RecommendedLearningServiceTests.cs
          
            Show resolved
            Hide resolved
        
      | [TestCase(2.4, 184.6)] | ||
| [TestCase(5, 195)] | ||
| public async Task | ||
| GetRecommendedLearningForSelfAssessment_returns_correct_recommendation_score_for_resource_with_learning_hub_ratings_only( | 
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.
This test title mentions "...correct score for {factor} only" but the expected scores seem to be higher than just the individual factor's contribution, so might be worth renaming.
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.
This was a consequence of me not realising the relationship was 1-1-0 (The Single comment above)
We now have 2 basic situations, either we can only calc the score from the LH rating, or we can have the LH rating + attempt both other values (req adj may be 0).
Restored these back to just the LH calculation, dropping the QuestionParameters setup.
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.
Ah I see, the case where no QuestionParameters are set up is handled here. I think given that that case is specifically considered in the logic for determining what to hide, it might be good to specify it in the title here then. Just will make it easier to see that it has indeed be tested from a quickly glance.
Maybe something like "...for_resource_only_with_learning_hub_rating_and_no_question_parameters"?
        
          
                DigitalLearningSolutions.Data.Tests/Services/RecommendedLearningServiceTests.cs
          
            Show resolved
            Hide resolved
        
      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.
Nice, think this covers everything now.
        
          
                DigitalLearningSolutions.Data/Services/RecommendedLearningService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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-711
Description
Filtering of resources where the delegate answer falls outside the range of results specified in CompetencyResourceAssessmentQuestionParameters. Refactored and added new unit tests to accommodate these changes.
Screenshots
No visual changes
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: