Skip to content

Conversation

Phil-NHS
Copy link
Contributor

@Phil-NHS Phil-NHS commented Jul 18, 2025

JIRA link

TD-5829

Description

I've created a Shared Project. This project is for the purpose of sharing coding across project in the solution and is intended to in future be used instead of internal model packages. Changes should not affect the App but should allow and encourage future development to share more code resources across projects. It should also ultimately reduce duplication in the code base.

Interface are used, including marker interfaces, for specifying configurations of services being injected, and to allow injected services to be providers for multiple more specific interfaces enabling services to have less knowledge and access to methods and data they do not use.

Settings and other configuration are being split with an interface whilst maintain the current appsettings structure. This is so if only non secure data is needed in a service then only that is injected, which also supports it being useable clientside safely.
E.g. Something like:

		interface IFindwisePublicSettings { string ApiUrl { get; } }
		interface IFindwisePrivateSettings { string ApiKey { get; } }
		IFindWiseSettings : IFindwisePublicSettings, IFindwisePrivateSettings

It seems alot of files have been moved but this is due to them all being connected to the SearchService.

I've included in the Shared Project

But I assume ultimately we would migrate from it to shared but that it would not be achievable to start without it.

I recommending starting with the Shared SearchService, then PublicSettings, then HttpClients

Several recommendations and points of discussions come from this task.

  • AppSettings be broken into more discrete classes would mean we can inject only configuration data required into services
  • Chosing libraries that are system based over aspnetcore makes them more shareable where the same functionality can be provided
  • Comments referring to classes not used via cref results in intellisense adding that using statement. I think this would be better avoided /// The htmlhelper.
  • Recommendation: Future tasks of unifying code into the shared project. Initially we now have lots of code in Shared that is similar to various other projects code. A task per folder to review opportunities to unify it may be beneficially. E.g. Http, Configuration, Helpers
  • I had to include cachingservice in the SharedProject to support SearchService, but its kind of infrastructure and makes me feel the have internal api wrappers, which potentially did all the cache and bearer keys would create nice segregation from infastructure.
  • potential contenders for being shared across more projects IProvider, ResourceAccessHelper, BaseService?, BaseHttpClient
  • i think the documentation to go with this should have some recommendation of when moving into shared project what should happen to services, my recommendation is review them and use the right libraries and the most specific interfaces to inject, so they are clearly defined especially thinking about if sensitive configuration settings should go into them.

Screenshots

NA


Developer checks

(Leave tasks unticked if they haven't been appropriate for your ticket.)

I have:

  • Run the IDE auto formatter on all files I’ve worked on and made sure there are no IDE errors relating to them
  • ** Ran style cop code clean up one, it changed every single file in the project and broke loads of stuff what do we ussually do for this??**
  • Written or updated tests for the changes (accessibility ui tests for views, tests for controller, data services, services, view models created or modified) and made sure all tests are passing
  • Manually tested my work with and without JavaScript (adding notes where functionality requires JavaScript)
  • Tested any Views or partials created or changed with Wave Chrome plugin. Addressed any valid accessibility issues and documented any invalid errors
  • Updated my Jira ticket with testing notes, including information about other parts of the system that were touched as part of the MR and need to be tested to ensure nothing is broken
  • [x ] Scanned over my pull request in GitHub and addressed any warnings from the GitHub Build and Test checks in the GitHub PR ‘Files Changed’ tab
    Either:
  • Documented my work in Confluence, updating any business rules applied or modified. Updated GitHub readme/documentation for the repository if appropriate. List of documentation links added/changed:
  • Confirmed that none of the work that I have undertaken requires any updates to documentation

@Phil-NHS Phil-NHS requested review from AnjuJose011 and binon July 18, 2025 15:27
@Phil-NHS Phil-NHS self-assigned this Jul 18, 2025
/// that secure or private configuration data is not inadvertently exposed to clients.
/// </para>
/// </summary>
public class PublicSettings : IPublicSettings
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Settings was broken up into more classes/objs in the appsettings we could inject just the bits we are using, -> least knowledge and interface segregation. But I expect there would be alot involved in doing this so I am expect appsetting will stay the same and we will inject PublicSettings and have PublicX per obj, (public may be the wrong word choice).

{
using LearningHub.Nhs.Models.Enums;
using Microsoft.AspNetCore.Mvc.Rendering;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not using this, it is only for comments here, i think we shouldnt include it. Or use System.X libraries so we don't pull through unneeded dependencies which arn't support in blazor for example, but generally more generic, more framework-neutral options are better for compatibility.

/// </summary>
public string LearningHubApiUrl { get; set; }

public IFindwiseSettingsPublic FindwiseSettings { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so we map the reduced FindwiseSettings and get just what we have defined as safe. My preference would be not putting PublicSettings into SearchService but inject two separate interface with just the information it needs. this may not be correct.

/// <summary>
/// Gets the base URL of the API.
/// </summary>
string ApiUrl { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other versions of this functionality in other project dont have APIUrl this comes from the baseHttpClient

/// The abstract api http client.
/// </summary>
public abstract class BaseHttpClient
public abstract class BaseHttpClient : IAPIHttpClient
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allow blazor to inject clients into services in the same way MVC does by using the interface.

@AnjuJose011 AnjuJose011 changed the base branch from RC to release-v2.6.1-Ivory July 30, 2025 10:03
@Phil-NHS Phil-NHS force-pushed the Develop/Feature/TD-5829-Introduce-and-Integrate-a-Shared-Project-into-the-LH-Solution branch from 6ac95a8 to 32ab570 Compare July 30, 2025 13:45
@Phil-NHS Phil-NHS closed this Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant