- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
HEEDLS-672 Learning Hub API client #756
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.
I think there are some improvements that could be made to tidy up the url handling.
        
          
                ...lLearningSolutions.Data/Models/LearningHubApiClient/ResourceReferenceWithReferenceDetails.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | return result; | ||
| } | ||
| 
               | 
          ||
| private static string GetUrlWithSearchQueryParams( | 
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'm pretty sure there must be a better way to do all this query string construction without using a bunch of string concatenation and interpolation.
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 initially used QueryHelpers.AddQueryString to add the query strings, but this method only accepts string values, not StringValues values, so it was impossible to add the query strings dealing with enumerables. Steve suggested I create the query strings manually, but if you can think of a better way, please enlighten me. It does feel a bit icky
        
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/DigitalLearningSolutions.Data.csproj
              
                Outdated
          
            Show resolved
            Hide resolved
        
      24b9d8f    to
    03ac5cd      
    Compare
  
    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.
Might be worth having a chat about/pairing on some ideas to sort out the query strings. I think it certainly should be possible to simplify some of the steps by being a bit smarter about whether we are appending & to the strings or not and making better use of the fact we've got IEnumerables we can use to join/split etc.
        
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Models/LearningHubApiClient/BulkResourceReferences.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Models/LearningHubApiClient/ResourceMetadata.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.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.
Looking good overall, just a couple of minor changes.
        
          
                DigitalLearningSolutions.Data/Helpers/ExternalApis/LearningHubApiClient.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | public async Task<BulkResourceReference> GetBulkResourcesByReferenceIds( | ||
| IEnumerable<int> resourceReferenceIds | 
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.
Given that an empty collection of ints could be passed in, and we know that we will get empty lists backs, we can probably bypass the unnecessary call and just return the model with the empty lists (or maybe null if you think that might be better)
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.
We should keep the return type correct (so no null), but I'm not sure whether we should short circuit the API call or not. I kind of feel like the API client should always call the API, and perhaps any calling method should be deciding whether it should call the API or not.
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.
Yeah, that sounds reasonable, it can stay as it is.
JIRA link
HEEDLS-672
HEEDLS-672
Description
Created the Learning hub Api client and associated view models. Configured user secrets to store the API key, and each developer will have to add it to their user secrets file locally.
Screenshots
None
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: