Skip to content
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.

Sign up for GitHub

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

Jump to bottom

QF-1140 Search again for company number and charity number matches #1469

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

QF-1140 Search again for company number and charity number matches #1469

wants to merge 15 commits into from

Conversation

ChrisJWoodcock
Copy link
Contributor

when searching for organisations during the Apply journey

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

SawNawDfE
SawNawDfE suggested changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@SawNawDfE SawNawDfE left a comment

Choose a reason for hiding this comment

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

I've suggested a few refactors in the tests and code. I'm still looking since there's a lot to go through. I will add more comments as I uncover more things

src/SFA.DAS.AssessorService.Application.Api.UnitTests/MappingBootstrapper.cs Show resolved Hide resolved
....Application.Api.UnitTests/Orchestrators/Organisation/Search/WhenOrganisationSearchByEpao.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Helpers/RegexHelper.cs Outdated Show resolved Hide resolved
...ce.Application.Api.UnitTests/Orchestrators/Organisation/Search/OrganisationSearchTestBase.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Orchestrators/OrganisationSearchOrchestrator.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Orchestrators/OrganisationSearchOrchestrator.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Orchestrators/OrganisationSearchOrchestrator.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Orchestrators/OrganisationSearchOrchestrator.cs Outdated Show resolved Hide resolved
src/SFA.DAS.AssessorService.Application.Api/Orchestrators/OrganisationSearchOrchestrator.cs Outdated Show resolved Hide resolved
...rs/Organisation/Search/WhenOrganisationSearchByNameOrCharityNumberOrCompanyNumberIsCalled.cs Outdated Show resolved Hide resolved
@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@SawNawDfE
Copy link
Contributor

I think it's a good idea to use the internal access modifier for new classes instead of public. There's no reason to allow other assemblies to access them, the test assemblies can access them using the InternalsVisibleTo attribute,, and it can make refactoring easier since you can rest assured that no external code is referencing your internal methods.

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@sonarcloud
Copy link

sonarcloud bot commented Mar 23, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 47 Code Smells

0.0% 0.0% Coverage
7.8% 7.8% Duplication

@ChrisJWoodcock
Copy link
Contributor Author

I think it's a good idea to use the internal access modifier for new classes instead of public. There's no reason to allow other assemblies to access them, the test assemblies can access them using the InternalsVisibleTo attribute,, and it can make refactoring easier since you can rest assured that no external code is referencing your internal methods.

Tried doing that but there was only 1 class in the end which looked like a candidate and internalizing that and adding the attribute just caused a problem with inconsistent accessibly in the test project

SawNawDfE
SawNawDfE previously approved these changes Mar 28, 2023
View reviewed changes
@das-github-app
Copy link

das-github-app bot commented May 4, 2023

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@sonarcloud
Copy link

sonarcloud bot commented May 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 47 Code Smells

0.0% 0.0% Coverage
7.8% 7.8% Duplication

@PeterDavidWilliams
Merge branch 'master' into QF-1140_SearchForCompanyOrCharityNumbers
2d4d7c2 
# Conflicts:
#	src/SFA.DAS.AssessorService.Application.Api/Infrastructure/IReferenceDataApiClient.cs
#	src/SFA.DAS.AssessorService.Application.Api/StartupConfiguration/Startup.cs
@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@SawNawDfE SawNawDfE requested review from SawNawDfE and removed request for SawNawDfE June 15, 2023 10:17
@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@das-github-app
Copy link

Please remember to check any packages used by this application to ensure they are up to date @ChrisJWoodcock. cc/ @ecarroll95

@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 17 Code Smells

0.0% 0.0% Coverage
2.5% 2.5% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SawNawDfE SawNawDfE SawNawDfE left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ChrisJWoodcock ChrisJWoodcock

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChrisJWoodcock @SawNawDfE @PeterDavidWilliams