[PM-26967] Added new metric properties

[voommen-livefront]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26967

📔 Objective

Add properties for metrics within the Repo
Allow the controller to accept metric values and update the database

📸 Screenshots

None

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @voommen-livefront's task —— View job

---

Code Review Complete

I've thoroughly reviewed the changes in this PR. Here's my comprehensive assessment:

---

Summary of Changes Since Last Review

The most recent commit (d91f92c) addressed SQL formatting issues in both stored procedure files by ensuring proper formatting with square brackets and line breaks as per Bitwarden's code style guidelines.

Status of Previously Identified Issues

✅ Fixed Issues

1. SQL Formatting - The SQL formatting issues raised by @mkincaid-bw have been properly addressed with correct bracket usage and line breaks
2. Unnecessary OrganizationId in SQL - The OrganizationReport_UpdateMetrics stored procedure correctly removed the organizationId parameter (only uses @Id)
3. Removal of Default Values - Most explicit default values (= 0, = string.Empty) have been removed from nullable properties

⚠️ Remaining Issues from Previous Reviews

Based on the review comments, there are still some concerns about nullable handling:

1. OrganizationReportMetricsRequest.cs:8-30 - All properties explicitly set = null, which is redundant for nullable types in C#
2. OrganizationReportResponseModel.cs:16-17 - CreationDate and RevisionDate explicitly set = null, which is unnecessary

---

Critical Issues Found

1. ⚠️ Redundant Null Assignments in Request Models

File: src/Core/Dirt/Reports/ReportFeatures/Requests/OrganizationReportMetricsRequest.cs:8-30

All 12 metric properties have explicit = null assignments:

public int? ApplicationCount { get; set; } = null;
public int? ApplicationAtRiskCount { get; set; } = null;
// ... etc

Issue: In C#, nullable types default to null without explicit assignment. This is redundant and goes against C# best practices and reviewer feedback.

Recommendation: Remove all = null assignments:

public int? ApplicationCount { get; set; }
public int? ApplicationAtRiskCount { get; set; }

---

2. ⚠️ Redundant Null Assignments in Response Model

File: src/Api/Dirt/Models/Response/OrganizationReportResponseModel.cs:16-17

public DateTime? CreationDate { get; set; } = null;
public DateTime? RevisionDate { get; set; } = null;

Issue: Same as above - nullable types default to null without explicit assignment.

Recommendation: Remove the = null assignments.

---

3. ❌ Potential Default Value Masking Issue

File: src/Core/Dirt/Reports/ReportFeatures/AddOrganizationReportCommand.cs:38

var requestMetrics = request.Metrics ?? new OrganizationReportMetricsRequest();

Issue: When request.Metrics is null, this creates a new instance with all properties being null. However, the subsequent code directly assigns these nullable values to the entity. This is actually correct behavior now that the redundant = null is being removed from OrganizationReportMetricsRequest. But if those defaults remain, this could mask the distinction between "no metrics provided" vs "metrics provided with zero values".

Current State: This is acceptable IF the redundant = null assignments are removed from OrganizationReportMetricsRequest.

---

Suggested Improvements

1. 🎨 Test Coverage for UpdateMetricsAsync

File: test/Infrastructure.EFIntegration.Test/Dirt/Repositories/OrganizationReportRepositoryTests.cs:494-533

Good: There's a test for UpdateMetricsAsync which verifies all metric properties are updated correctly.

Suggestion: Consider adding test cases for:

• Updating metrics with null values (to ensure nulls are properly persisted)
• Updating only some metrics while leaving others unchanged
• Verifying that RevisionDate is updated during metrics update

---

2. 🎨 Missing Unit Tests for New Classes

Missing Coverage:

• OrganizationReportMetricsData.cs - The From() static factory method has no unit tests
• OrganizationReportMetricsRequest.cs - No tests for JSON serialization/deserialization with the custom JsonPropertyName attributes

Recommendation: Add unit tests for:

// Test that From() correctly maps all properties
// Test that From() with null request returns object with only OrganizationId set
// Test JSON property name mapping works correctly

---

3. 🎨 Null Safety Consideration in Repository

Files:

• src/Infrastructure.Dapper/Dirt/OrganizationReportRepository.cs:178-203
• src/Infrastructure.EntityFramework/Dirt/Repositories/OrganizationReportRepository.cs:189-214

Both repositories have UpdateMetricsAsync methods that directly pass nullable metric values to the stored procedure/database update.

Consideration: The stored procedures expect INT (non-nullable) parameters but C# is passing int? values. While Dapper/EF will handle this by converting null to SQL NULL (which will be rejected by the stored procedure), this mismatch should be documented or handled explicitly.

Current behavior: If any metric is null, the stored procedure will fail because it expects non-null INT values.

Recommendation: Either:

1. Make the stored procedure parameters nullable (@ApplicationCount INT = NULL), OR
2. Add validation in the command handler to ensure metrics are not null before calling UpdateMetricsAsync, OR
3. Add XML documentation to clarify that null values will cause the operation to fail

---

4. 🎨 Inconsistent Using Directive Placement

File: src/Infrastructure.EntityFramework/Dirt/Repositories/OrganizationReportRepository.cs:189-214

The UpdateMetricsAsync method uses using (var scope = ...) but doesn't await the operation inside the using block, returning the Task directly.

Current Code:

public Task UpdateMetricsAsync(Guid reportId, OrganizationReportMetricsData metrics)
{
    using (var scope = ServiceScopeFactory.CreateScope())
    {
        var dbContext = GetDatabaseContext(scope);
        
        return dbContext.OrganizationReports
            .Where(p => p.Id == reportId)
            .UpdateAsync(p => new Models.OrganizationReport { ... });
    }
}

Issue: The using statement will dispose of the scope before the async operation completes.

Recommendation: Make the method async and await inside the using:

public async Task UpdateMetricsAsync(Guid reportId, OrganizationReportMetricsData metrics)
{
    using (var scope = ServiceScopeFactory.CreateScope())
    {
        var dbContext = GetDatabaseContext(scope);
        
        await dbContext.OrganizationReports
            .Where(p => p.Id == reportId)
            .UpdateAsync(p => new Models.OrganizationReport { ... });
    }
}

---

Good Practices Observed

• ✅ Comprehensive integration tests added for the new UpdateMetricsAsync functionality
• ✅ Proper use of nullable types throughout the data models
• ✅ SQL stored procedures follow Bitwarden's formatting standards
• ✅ Consistent property naming across request/data/entity models
• ✅ JSON property name attributes for API contract clarity

---

Action Items for Author

High Priority

1. ❌ Remove redundant = null assignments from OrganizationReportMetricsRequest.cs (all 12 properties)
2. ❌ Remove redundant = null assignments from OrganizationReportResponseModel.cs (lines 16-17)
3. ❌ Fix the using scope issue in OrganizationReportRepository.UpdateMetricsAsync (Entity Framework implementation)

Medium Priority

4. 📝 Add unit tests for OrganizationReportMetricsData.From() method
5. 📝 Add tests for JSON property name mapping in OrganizationReportMetricsRequest
6. 📝 Consider adding validation or documentation about null handling in UpdateMetricsAsync

Low Priority

7. 🎨 Consider adding test cases for partial metric updates and null value handling

---

Additional Context: Nullable Type Defaults in C#

In C#, when you declare a nullable type like int? or DateTime?, it automatically defaults to null without explicit assignment. The syntax:

public int? MyProperty { get; set; } = null;

Is functionally identical to:

public int? MyProperty { get; set; }

The explicit = null is redundant and considered poor practice because:

1. It adds unnecessary code
2. It suggests to readers that there's something special about this null default
3. It goes against C# conventions and best practices
4. Multiple reviewers have specifically requested its removal

---

[codecov]

Codecov Report

❌ Patch coverage is 62.96296% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.82%. Comparing base (5f0e038) to head (d91f92c).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ucture.Dapper/Dirt/OrganizationReportRepository.cs	0.00%	24 Missing ⚠️
.../Dirt/Repositories/OrganizationReportRepository.cs	0.00%	23 Missing ⚠️
.../Dirt/Models/Data/OrganizationReportMetricsData.cs	83.78%	5 Missing and 1 partial ⚠️
...Models/Response/OrganizationReportResponseModel.cs	89.28%	2 Missing and 1 partial ⚠️
.../Dirt/Controllers/OrganizationReportsController.cs	91.66%	0 Missing and 1 partial ⚠️
...rts/ReportFeatures/AddOrganizationReportCommand.cs	93.33%	0 Missing and 1 partial ⚠️
.../UpdateOrganizationReportApplicationDataCommand.cs	0.00%	0 Missing and 1 partial ⚠️
...Features/UpdateOrganizationReportSummaryCommand.cs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6519      +/-   ##
==========================================
+ Coverage   51.86%   55.82%   +3.95%     
==========================================
  Files        1901     1905       +4     
  Lines       84051    84210     +159     
  Branches     7501     7510       +9     
==========================================
+ Hits        43594    47010    +3416     
+ Misses      38763    35423    -3340     
- Partials     1694     1777      +83     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[withinfocus]

Quick fly-by. I am out of the office for a bit so please re-request team review after updates.

Claude also pointed out several things.

[withinfocus]

🎨 I wouldn't want defaults on these, and null is not empty (use null). Those strings are nullable too, as are the ints -- why not represent the true default? I wouldn't want code later down the line to see a zero and indicate that's a positive when it's just undefined for example.

[withinfocus]

Take a look at all the nullable usage across the PR too.

[voommen-livefront]

updated the usage of nulls. Only using string.Empty where it is required in the code. You are correct - when a request is made, most values may be null.

[withinfocus]

❌ If you have an ID for this exact row I don't see any point in the organization ID being used. I left similar comments in the past -- auth is out of scope for this and the rights to change the row should already be established.

[voommen-livefront]

Thanks for catching this!

[withinfocus]

The DB changes are fine now but the not-owned-by-me changes still have concerns.

[withinfocus]

⚠️ It still doesn't seem right to default these to me.

[voommen-livefront]

I removed the defaults - this time even more carefully reading through the PR - and I think I caught everything, excetpt those that were required by the ITableObject

[prograhamming]

I agree that these should be changed. I would make PasswordCount, PasswordAtRiskCount, and MemberCount to have type int? and remove the null-coalescing. Defaulting this to zero creates ambiguity: does 0 mean "we calculated and found zero passwords" or does it mean "we haven't calculated this yet?"

[withinfocus]

⚠️ Also should not be set.

[voommen-livefront]

here too - I have removed it as much as possible, except when I absolutely needed it.

[prograhamming]

I would make the OrganizationReportMetricRequest nullable and remove it being set.

[withinfocus]

⚠️ Also should not be set.

[voommen-livefront]

Thanks for catching this.

[prograhamming]

Let's remove setting null. By default in C# value types (e.g. int, bool, etc.) cannot be null. By using the ? operator we are making this a nullable type which allows it to hold null in addition to its normal values. It is really syntactic sugar for Nullable<int> which wraps the value type in a struct that tracks whether a value exists.

[mkincaid-bw]

Couple minor formatting issues.

[mkincaid-bw]

⛏️ Per the Code Style docs, UPDATE and the table name should be on separate lines.

Also, all object names should include square brackets, ie:

UPDATE 
    [dbo].[OrganizationReport]
SET
    [ApplicationCount] = @ApplicationCount,
    [ApplicationAtRiskCount] = @ApplicationAtRiskCount,
...
WHERE 
    [Id] = @Id

[voommen-livefront]

updated formatting. Sorry I missed this earlier.

[mkincaid-bw]

Same as above.

[mkincaid-bw]

DB Changes LGTM