Fix/issue-2870 [Team Page] API returns 200 OK and saves leading spaces in name instead of trimming/validation error by Valience · Pull Request #3484 · ita-social-projects/VictoryCenter-Back

Valience · 2026-08-20T20:09:32Z

Issue ticket link

2870

Description

Fixes API accepting leading/trailing whitespace in TeamCategory name and description fields.
Previously, PUT /api/TeamCategories/{id} returned 200 OK and persisted values with surrounding
whitespace. Now returns 400 Bad Request with a validation error.

How it looks

Summary of change

ErrorMessagesConstants.cs: Added PropertyMustNotHaveLeadingOrTrailingSpaces(string property).
BaseTeamCategoryValidator.cs: Added .Must(...) rule to Name and Description rejecting
values that differ from their trimmed form.
BaseTeamCategoryValidatorTests.cs: Added 4 tests covering leading-space, trailing-space,
both, and the no-space passing case for both fields.

How to recreate changes

Send PUT /api/TeamCategories/{id} with name or description containing a leading/trailing
space.
Before fix: 200 OK, value persisted with whitespace.
After fix: 400 Bad Request, validation error returned.

Check List

New tests was added or existing was modified
Changes has severe impact on users
Changes has medium impact on users
Changes has light impact on users
PR meets all conventions

Summary by CodeRabbit

Bug Fixes
- Team category names and descriptions now reject leading or trailing spaces.
- Validation messages clearly identify fields that contain surrounding whitespace.
Tests
- Added coverage for invalid names and descriptions with surrounding spaces.
- Confirmed properly formatted values continue to pass validation.

coderabbitai · 2026-08-20T20:09:39Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b07b1419-7e7b-4a8b-b1ac-4af1f45bc26b

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a1ad861-9cd5-4225-a95a-296a41524266

📥 Commits

Reviewing files that changed from the base of the PR and between 6f087ab and 1e9644b.

📒 Files selected for processing (3)

VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs
VictoryCenter/VictoryCenter.BLL/Validators/TeamCategories/BaseTeamCategoryValidator.cs
VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/TeamCategories/BaseTeamCategoryValidatorTests.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The change adds a reusable validation message and rejects leading or trailing spaces in team category names and descriptions. Unit tests cover leading, trailing, combined, and valid spacing cases.

Changes

Team category whitespace validation

Layer / File(s)	Summary
Whitespace validation rules `VictoryCenter/VictoryCenter.BLL/Constants/ErrorMessagesConstants.cs`, `VictoryCenter/VictoryCenter.BLL/Validators/TeamCategories/BaseTeamCategoryValidator.cs`	Adds the whitespace validation message. Applies the rule to team category names and descriptions.
Whitespace validation tests `VictoryCenter/VictoryCenter.UnitTests/ValidatorsTests/TeamCategories/BaseTeamCategoryValidatorTests.cs`	Tests leading, trailing, combined, and valid spacing cases for names and descriptions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to 1e964

The new whitespace validation may crash when Name or Description is null, returning a 500 response instead of a 400 validation error. Merge should wait for null-safe predicates or explicit cascade behavior.

Suggested reviewers: maxvonlancaster, nazartymoshchuk

Poem

Spaces at the edges now leave,
Names and descriptions validate with ease.
A clear message marks the line,
Tests keep every case in time.
Clean inputs pass the gate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description explains the issue, summarizes the change, provides reproduction steps, includes screenshots, links the ticket, and includes a checklist.
Title check	✅ Passed	The title clearly identifies issue `#2870` and the main fix for leading spaces in the Team Page API.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🛠️ Fix failing CI checks

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/issue-2870

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

mehalyna

The identical Must(...) + WithMessage(...) pair is copy-pasted for both properties. Given points A1/A2, this rule will be needed on several more DTOs; the repo already has a precedent for factoring such rules out (EventNewsCategoryNameValidationExtensions.cs). Worth extracting before it is copied a third time.
Min/max are evaluated on the raw string before the space rule. A 20-char name plus one trailing space (MaxNameLength = 20, TeamCategoryConstants.cs) fails with "maximum length of 20 characters" rather than the space message, because cascade stops at MaximumLength. Not incorrect - the request is rejected either way - but the error tells the admin the wrong thing to fix. Not covered by any test.
Tests geps

No API-level test. The issue is literally "API returns 200 OK", yet there is no test in ControllerTests/TeamCategories/ asserting 400 for a padded name.
No case for non-space whitespace (\t, \n, \u00A0). string.Trim() strips all of these, so the rule rejects them - that behaviour is untested and could silently change.
No case for the interaction in padded value whose trimmed length is below MinNameLength, e.g. " abc "
the two ShouldNotHaveError tests build the exact same valid DTO as the pre-existing Validate_ValidDto_ShouldNotHaveErrors at line 141; they add no coverage.

mehalyna · 2026-08-21T14:44:13Z

                nameof(CreateTeamCategoryDto.Name),
-                TeamCategoryConstants.MaxNameLength));
+                TeamCategoryConstants.MaxNameLength))
+            .Must(name => name == name.Trim())


Must predicate is null-unsafe; safety comes only from a global static setting
BaseTeamCategoryValidator.cs Line 23 and 39

Name/Description are string ... = null! (CreateTeamCategoryDto.cs), so a request omitting the field yields null. FluentValidation's length validators skip nulls, but Must does not - the predicate dereferences name and throws NullReferenceException.

Today this does not blow up, because rule-level cascade is globally forced to Stop in two places (ServicesConfiguration.cs lines 110, 128) and, for tests, by a [ModuleInitializer] (ValidatorsConfig.cs line 11), so NotEmpty() short-circuits the chain. That is an implicit, action-at-a-distance guarantee: ~30 other validators in this repo defend explicitly with .Cascade(CascadeMode.Stop) (e.g. BaseCompanyProfileContactDtoValidator.cs:12), this one does not. Any consumer that resolves the validator without that global (new test assembly, library reuse, someone flipping the default) turns a 400 into a 500.

Make the rule chain state its own cascade, or write the predicate so it tolerates null.

mehalyna · 2026-08-21T14:45:37Z


+    public static string PropertyMustNotHaveLeadingOrTrailingSpaces(string property)
+    {
+        return $"{property} must not have leading or trailing spaces.";


Message text ends with a period; no other message in that file does (compare lines 126, 131, 136). These strings surface in the admin UI, so the punctuation is user-visible.

mehalyna · 2026-08-21T14:52:05Z

The build is failing due to missing constructor parameters in the PDF report handler test files. The handlers now require a hubContext parameter that the tests are not providing.

Root Cause

Looking at the compilation errors:

error CS7036: There is no argument given that corresponds to the required parameter 'hubContext'

This error appears in:

UpdatePdfReportHandlerTests.cs(437,9)
CreatePdfReportHandlerTests.cs(176,9)
DeletePdfReportHandlerTests.cs(173,9)
ReorderPdfReportsTests.cs (multiple lines)

The handler constructors have been updated to include a hubContext parameter (likely for SignalR notifications), but the test factories haven't been updated to pass this parameter.

Solution

Update the handler instantiation in all PDF report test files to include the missing hubContext parameter.

For `CreatePdfReportHandlerTests.cs` (line 175-181):

private CreatePdfReportHandler CreateHandler() =>
    new(
        _mockRepositoryWrapper.Object,
        _mockPdfService.Object,
        _validator,
        _mockMapper.Object,
        _mockReorderService.Object,
        Mock.Of<IHubContext<YourHubName>>());  // Add this line

For `UpdatePdfReportHandlerTests.cs` (line 436-437):

private UpdatePdfReportHandler CreateHandler() =>
    new(_mockRepositoryWrapper.Object, _validator, _mockMapper.Object, 
        Mock.Of<IHubContext<YourHubName>>());  // Add this parameter

For `DeletePdfReportHandlerTests.cs` and `ReorderPdfReportsTests.cs`:

Apply the same pattern - add a mock IHubContext parameter when instantiating the handlers.

Note: Replace YourHubName with the actual SignalR Hub name used in your application (likely PdfReportHub or similar based on the context).

hae-ctr-mykhailo-klapchuk

Check Halyna's comments

coderabbitai · 2026-08-24T23:18:09Z

✅ Committed CI fixes to this branch (9be82d8). CI will re-run on the new commit.

Note: Fixing CI failures is a beta feature and may encounter errors. Expect some limitations and changes as we gather feedback and continue to improve it.

ℹ️ Local verification: tests skipped.

1 PR-caused check(s)

GitHub Actions: SonarQube / Build and analyze

⏭️ 1 check(s) skipped — already failing on `release/1.0.0` (not caused by this PR)

GitHub Actions: SonarQube / 0_Build and analyze.txt

4 file(s) modified

VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/CreatePdfReportHandlerTests.cs
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/DeletePdfReportHandlerTests.cs
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/ReorderPdfReportsTests.cs
VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/UpdatePdfReportHandlerTests.cs

View agent analysis

Confirms the pattern. Let's check the other two handlers and then all test files' constructors to add proper mock setup.

Now let's fix all 4 test files by adding proper mock setup for `Clients.All`.

All four PdfReports test files now properly mock `IHubContext<PdfReportsHub>.Clients.All` so `SendAsync` no longer throws `NullReferenceException`.

## Summary

**Root cause:** The PR's diff added an `IHubContext<PdfReportsHub>` parameter/mock to the four `PdfReports` handler test classes (`CreatePdfReportHandlerTests`, `DeletePdfReportHandlerTests`, `UpdatePdfReportHandlerTests`, `ReorderPdfReportsTests`), created via `new Mock<IHubContext<PdfReportsHub>>()` with no further setup. All four handlers call `_hubContext.Clients.All.SendAsync(...)`. Since Moq returns `null` for the unconfigured `Clients` property (it's not a `Task`-returning member so Moq's task-auto-completion doesn't apply), dereferencing `.All` on that `null` throws `NullReferenceException`, matching the exact stack traces in the CI log (`DeletePdfReportHandler.cs:83`, `CreatePdfReportHandler.cs:101`, `UpdatePdfReportHandler.cs:74`, `ReorderPdfReportsHandler.cs:72`).

This is unrelated to the TeamCategory whitespace-validation intent of the PR — it's an incidental test-setup bug introduced by the same commit.

**Fix:** In each of the 4 test files' constructors, after creating `_mockHubContext`, added:
```csharp
var mockClientProxy = new Mock<IClientProxy>();
var mockClients = new Mock<IHubClients>();
mockClients.Setup(c => c.All).Returns(mockClientProxy.Object);
_mockHubContext.Setup(h => h.Clients).Returns(mockClients.Object);
```
This makes `_hubContext.Clients.All` resolve to a valid mocked `IClientProxy`, whose `SendAsync` (via Moq's default `Task`-returning behavior) completes without throwing.

Files changed:
- `VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/CreatePdfReportHandlerTests.cs`
- `VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/DeletePdfReportHandlerTests.cs`
- `VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/UpdatePdfReportHandlerTests.cs`
- `VictoryCenter/VictoryCenter.UnitTests/MediatRHandlersTests/PdfReports/ReorderPdfReportsTests.cs`

I did not touch the TeamCategory validator/tests since those weren't part of the reported failures and their diff appears self-consistent with the PR's stated intent. The remaining SonarQube warnings in the log (line-length, cyclomatic complexity, etc.) are pre-existing informational warnings, not part of the build/test failure, and are out of scope for this fix.

Co-Authored-By: CodeRabbit <noreply@coderabbit.ai>

sonarqubecloud · 2026-08-25T23:04:51Z

Quality Gate failed

Failed conditions
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

fix: add validation for leading/trailing spaces

1e9644b

mehalyna requested review from NazarTymoshchuk, maxvonlancaster and mehalyna August 21, 2026 14:03

mehalyna reviewed Aug 21, 2026

View reviewed changes

hae-ctr-mykhailo-klapchuk requested changes Aug 24, 2026

View reviewed changes

Valience added 2 commits August 25, 2026 00:01

Merge branch 'release/1.0.0' into fix/issue-2870

07ed4dd

fix: fix comments

8ca5acd

Valience requested a deployment to stage August 24, 2026 23:04 — with GitHub Actions Waiting

Fix 1 failing CI check(s): GitHub Actions: SonarQube / Build and analyze

9be82d8

Co-Authored-By: CodeRabbit <noreply@coderabbit.ai>

coderabbitai Bot requested a deployment to stage August 24, 2026 23:23 Waiting

mehalyna removed the request for review from NazarTymoshchuk August 25, 2026 14:04

Valience and others added 2 commits August 25, 2026 21:17

Merge branch 'release/1.0.0' into fix/issue-2870

c3bdf44

fix: fix build error

4605a72

Valience requested a deployment to stage August 25, 2026 20:00 — with GitHub Actions Waiting

fix: build error #2

801caf2

Valience requested a deployment to stage August 25, 2026 22:47 — with GitHub Actions Waiting

Valience requested review from hae-ctr-mykhailo-klapchuk and mehalyna August 26, 2026 15:48

hae-ctr-mykhailo-klapchuk approved these changes Aug 27, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix/issue-2870 [Team Page] API returns 200 OK and saves leading spaces in name instead of trimming/validation error - #3484

Fix/issue-2870 [Team Page] API returns 200 OK and saves leading spaces in name instead of trimming/validation error#3484
Valience wants to merge 7 commits into
release/1.0.0from
fix/issue-2870

Valience commented Aug 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

mehalyna left a comment •

edited

Loading

Uh oh!

mehalyna Aug 21, 2026

Uh oh!

mehalyna Aug 21, 2026

Uh oh!

mehalyna commented Aug 21, 2026

Uh oh!

hae-ctr-mykhailo-klapchuk left a comment

Uh oh!

coderabbitai Bot commented Aug 24, 2026 •

edited

Loading

Uh oh!

sonarqubecloud Bot commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Valience commented Aug 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue ticket link

Description

How it looks

Summary of change

How to recreate changes

Check List

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

mehalyna left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehalyna Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

mehalyna Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

mehalyna commented Aug 21, 2026

Root Cause

Solution

For CreatePdfReportHandlerTests.cs (line 175-181):

For UpdatePdfReportHandlerTests.cs (line 436-437):

For DeletePdfReportHandlerTests.cs and ReorderPdfReportsTests.cs:

Uh oh!

hae-ctr-mykhailo-klapchuk left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud Bot commented Aug 25, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Valience commented Aug 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

mehalyna left a comment •

edited

Loading

For `CreatePdfReportHandlerTests.cs` (line 175-181):

For `UpdatePdfReportHandlerTests.cs` (line 436-437):

For `DeletePdfReportHandlerTests.cs` and `ReorderPdfReportsTests.cs`:

coderabbitai Bot commented Aug 24, 2026 •

edited

Loading