Skip to content

Remaining endpoint catalog work #229 - #230

Open
JbravoI wants to merge 18 commits into
kevbite:masterfrom
JbravoI:master
Open

JbravoI wants to merge 18 commits into
kevbite:masterfrom
JbravoI:master

Conversation

@JbravoI

@JbravoI JbravoI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Complete the remaining endpoint catalog work by implementing the missing Persons with Significant Control notifications endpoint and resolve the CI restore failure caused by a vulnerable SourceLink dependency. #229

Changes

  • Implemented the PSC notifications endpoint:
    • GET /company/{company_number}/persons-with-significant-control/{psc_id}/notifications
  • Added support for:
    • filter
    • items_per_page
    • start_index
  • Added notification response models and link models.
  • Added URI-builder and response deserialization tests.
  • Completed the remaining endpoint catalog work identified in item 2.
  • Removed Microsoft.SourceLink.GitHub 8.0.0, which introduced vulnerable Microsoft.Build.Tasks.Git 8.0.0.
  • Updated the .NET SDK from 10.0.100 to 10.0.303.
  • Pinned the Docker build image to .NET SDK 10.0.303.

Why

The Companies House API specification included a PSC notifications endpoint that was not exposed by the client. This PR completes the missing endpoint implementation and brings the endpoint catalog in line with the specification.

The CI pipeline was also failing during restore because NU1902 treated the vulnerable Microsoft.Build.Tasks.Git 8.0.0 package as an error. Removing the obsolete SourceLink package and updating the SDK resolves that restore failure.

Testing

  • dotnet restore passes successfully.
  • Added URI-builder tests covering encoded IDs, filtering, and pagination.
  • Added response deserialization tests for PSC notifications.
  • Confirmed the affected files have no editor diagnostics.
  • Full local test execution remains limited because the local api-enumerations submodule is empty. CI initializes submodules recursively during checkout.

@kevbite
kevbite requested review from kevbite and a balanced review from Copilot September 11, 2026 14:25
@kevbite kevbite self-assigned this Sep 11, 2026
Skip API scenarios without credentials

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Response-model specification mismatches and the unimplemented Docker SDK pin must be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds the missing PSC notifications endpoint and addresses the vulnerable SourceLink dependency.

Changes:

  • Adds PSC notification clients, URI construction, models, and tests.
  • Removes explicit SourceLink package references.
  • Updates the requested .NET SDK version.
File summaries
File Description
PersonsWithSignificantControlDetailsUriBuilderTests.cs Tests notification URI generation.
CompaniesHousePersonsWithSignificantControlDetailsClientTests.cs Tests notification deserialization.
SearchResponseDeserializationScenarioTests.cs Imports the fallback search model namespace.
PersonsWithSignificantControlDetailsUriBuilder.cs Builds notification endpoint URIs.
IPersonsWithSignificantControlDetailsUriBuilder.cs Exposes notification URI construction.
PersonWithSignificantControlNotifiedTo.cs Models notified company details.
PersonWithSignificantControlNotificationsLinks.cs Models list links.
PersonWithSignificantControlNotifications.cs Models the notification envelope.
PersonWithSignificantControlNotificationLinks.cs Models item links.
PersonWithSignificantControlNotification.cs Models notification items.
ICompaniesHousePersonsWithSignificantControlDetailsClient.cs Adds the endpoint contract.
CompaniesHousePersonsWithSignificantControlDetailsClient.cs Implements endpoint retrieval.
CompaniesHouseClient.cs Exposes the endpoint publicly.
CompaniesHouse.csproj Removes SourceLink dependency.
CompaniesHouse.Extensions.Microsoft.DependencyInjection.csproj Removes SourceLink dependency.
global.json Updates the requested SDK.
Directory.Packages.props Removes SourceLink version management.
Review details
  • Files reviewed: 20/20 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread global.json Outdated
@JbravoI

JbravoI commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

All review comments have been addressed in the latest updates:

  • Docker and global.json now use the same pinned .NET SDK version.
  • PSC notification date_of_birth is correctly modelled on the response envelope and covered by tests.
  • Notification kind uses the generated string-backed value type.
  • notified_to.company_status now uses CompanyStatus.

@kevbite could you please re-review and approve PR #230 when you have a moment?

Thanks

@kevbite

kevbite commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Can we bump the Microsoft.SourceLink.GitHub version to latest without the vulnerability so that we still get source links back for people using the code? - Have a feeling we'll need to copy in the .git folder to make it work as I think previously it just ignored it.

@JbravoI

JbravoI commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Can we bump the Microsoft.SourceLink.GitHub version to latest without the vulnerability so that we still get source links back for people using the code? - Have a feeling we'll need to copy in the .git folder to make it work as I think previously it just ignored it.

Thanks @kevbite, i`ll implemet it.

@JbravoI

JbravoI commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Hi @kevbite

I just implemented it in the new commit 7d29b88

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants