Skip to content

feat(badges): add name property to badge models and mock - #1155

Merged
msrouchou merged 1 commit into
ubisoft:mainfrom
jasonwoods-7:feat/badge-name
Oct 9, 2026
Merged

msrouchou merged 1 commit into
ubisoft:mainfrom
jasonwoods-7:feat/badge-name

Conversation

@jasonwoods-7

@jasonwoods-7 jasonwoods-7 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
  • GitLab badges support an optional name field. Expose it on Badge, BadgeCreate and BadgeUpdate so callers can set and read it.
  • Mock: store the name on Badge. Set it on create and update in the project and group badge clients.
  • Mock: add a BadgeCollection.Add(name, linkUrl, imageUrl) overload. Keep the old Add(linkUrl, imageUrl) overload so existing callers still compile.
  • Tests: cover name on create and update for project and group badges, in both the integration tests and the mock tests.

@jasonwoods-7
jasonwoods-7 requested a review from a team as a code owner October 7, 2026 17:16
@jasonwoods-7
jasonwoods-7 requested review from msrouchou and removed request for a team October 7, 2026 17:16
Comment thread NGitLab.Mock/BadgeCollection.cs
Comment thread NGitLab.Tests/ProjectBadgeClientTests.cs
@msrouchou

Copy link
Copy Markdown
Member

Thanks for the PR. A few points:

  1. Mock Update ignores Name. ProjectBadgeClient.cs (badgeToUpdate.LinkUrl = badge.LinkUrl;) and GroupBadgeClient.cs need badgeToUpdate.Name = badge.Name; too, otherwise the mock drops the name on update and differs from the real API. Please match how LinkUrl and ImageUrl treat null, and add tests in NGitLab.Mock.Tests and GroupBadgeClientTests.
  2. The description says the name is "required" in BadgeCollection.Add, but nothing validates it, and GitLab treats name as optional. Please fix the description, or add the check and drop the Is.Null assert.
  3. Please check that BadgeCreate.Name and BadgeUpdate.Name are omitted from the JSON when null, instead of sending "name": null.

* GitLab badges support an optional name field. Expose it on Badge, BadgeCreate and
  BadgeUpdate so callers can set and read it.
* Mock: store the name on Badge. Set it on create and update in the project and group badge
  clients.
* Mock: add a BadgeCollection.Add(name, linkUrl, imageUrl) overload. Keep the old
  Add(linkUrl, imageUrl) overload so existing callers still compile.
* Tests: cover name on create and update for project and group badges, in both the
  integration tests and the mock tests.
@jasonwoods-7

Copy link
Copy Markdown
Contributor Author

It looks like Claude fixed everything except point 3 in your review. I asked about that and it said

No. I found no test that checks serialized request JSON.

  • JsonConverterTests.cs only tests deserialization. It calls Serializer.Deserialize on JSON input.
  • The only Serialize call in NGitLab.Tests is in Docker/GitLabDockerContainer.cs:389. It serializes test credentials, not request bodies.
  • HttpRequestorTests.cs has no body or JSON checks.
  • The Docker tests (ProjectBadgeClientTests, GroupBadgeClientTests) send real requests, so they never see the request JSON.

So nothing checks that a null Name is left out. That behavior comes only from Serializer.cs:23 (DefaultIgnoreCondition = WhenWritingNull).

I could add a small test that serializes BadgeCreate and BadgeUpdate with a null Name and asserts there is no "name" key. It would back up your reply on the null-omit review point. Do you want it?

Would you like that added?

@msrouchou

Copy link
Copy Markdown
Member

Would you like that added?

nah it is fine, merging, thank you for the contribution.

@msrouchou
msrouchou merged commit c7ee11c into ubisoft:main Oct 9, 2026
8 checks passed
@msrouchou

Copy link
Copy Markdown
Member

@jasonwoods-7
jasonwoods-7 deleted the feat/badge-name branch October 9, 2026 20:44
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.

2 participants