Refactor: Remove access check in Preview handler and add unit tests f… - #175
Merged
AhmadRAbuhussein merged 1 commit intoJun 1, 2026
Merged
Conversation
…or Scriban JSON mapping - Removed the access check for Admin and Member roles in the Preview handler. - Added unit tests for NativeJSONMapper to ensure correct mapping using startup templates. - Introduced tests for RunMapperEnrichment to validate JSON parsing and enrichment logic. - Created ScribanGeneratorParityTests to ensure parity with the output of the Scriban generator. - Added ScribanJsonHelperArrayMappingTests to verify array mapping functionality. - Implemented ScribanJsonHelperErrorHandlingTests to check error handling for invalid templates. - Developed ScribanJsonHelperLookupAndTypeRuleTests to validate lookup and type rule mappings. - Added ScribanJsonHelperRootMappingTests to test root mapping functionality. - Created a helper class for rendering JSON templates and asserting JSON equality.
📝 WalkthroughWalkthroughThis PR removes an authorization check from the ChangesAuthorization Guard Removal
Scriban JSON Rendering Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
SW.Bitween.UnitTests/ScribanJsonHelperRootMappingTests.cs (1)
131-138: ⚡ Quick winTest does not validate partner/global typed mapping behavior
Line 134 hardcodes
nullfor both fields, so this test passes without exercising__partner__/__globals__mapping at all. Please use typed conversion expressions (or generated parity expressions) so the assertion actually guards the intended behavior.Proposed test adjustment
- var template = "{ \"partnerBoolTarget\": null, \"globalNumberTarget\": null }"; + var template = + "{ \"partnerBoolTarget\": {{ (__partner__?.flag == null ? null : (__partner__?.flag ? 1 : 0)) | json }}, " + + "\"globalNumberTarget\": {{ (__globals__?.S[\"K\"] | to_float) | json }} }"; var output = Render(template, input); - AssertJsonEquals("{\"partnerBoolTarget\":null,\"globalNumberTarget\":null}", output); + AssertJsonEquals("{\"partnerBoolTarget\":1,\"globalNumberTarget\":1.0}", output);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bitween.UnitTests/ScribanJsonHelperRootMappingTests.cs` around lines 131 - 138, The test PartnerAndGlobalTypedTargets_AreRepresentedAsNullTemplateLiteral currently hardcodes null in the template so it never exercises __partner__/__globals__ mapping; update the template string (the variable named template) to use typed conversion/parity expressions that reference the input's __partner__ and __globals__ properties (e.g., convert __partner__.flag to a typed template expression and __globals__.S.K to a typed numeric expression) so Render(template, input) actually triggers the partner/global mapping logic, then keep the AssertJsonEquals against the expected JSON produced by those typed conversions; use the Render method and AssertJsonEquals as-is to validate the behavior.SW.Bitween.UnitTests/ScribanJsonHelperLookupAndTypeRuleTests.cs (1)
115-121: ⚡ Quick winString-to-bool test is currently a tautology
Line 119 renders a literal
null, so this test never validates string-to-bool behavior. Replace it with the actual cast expression under test (with both valid/invalid string cases) to make the test meaningful.Proposed test adjustment
- var input = "{\"name\":\"x\"}"; - - var value = RenderValue("{{ null | json }}", input); + var input = "{\"name\":\"x\"}"; + var value = RenderValue( + "{{ (name == \"true\" ? true : (name == \"false\" ? false : null)) | json }}", + input); Assert.AreEqual(JTokenType.Null, value.Type);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bitween.UnitTests/ScribanJsonHelperLookupAndTypeRuleTests.cs` around lines 115 - 121, The test TypeRules_StringToBool_IsNull is rendering a literal null instead of exercising the string-to-bool cast; update the RenderValue call to use the actual cast/filter expression under test (replace "{{ null | json }}" with the cast expression you use to convert strings to bool) and add two assertions: one using a valid boolean string input (e.g. input with "name":"true") asserting JTokenType.Boolean, and one using an invalid string input (e.g. "name":"x") asserting JTokenType.Null; keep the test helper RenderValue and the method name unchanged so the test targets the same code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@SW.Bitween.UnitTests/ScribanJsonHelperLookupAndTypeRuleTests.cs`:
- Around line 115-121: The test TypeRules_StringToBool_IsNull is rendering a
literal null instead of exercising the string-to-bool cast; update the
RenderValue call to use the actual cast/filter expression under test (replace
"{{ null | json }}" with the cast expression you use to convert strings to bool)
and add two assertions: one using a valid boolean string input (e.g. input with
"name":"true") asserting JTokenType.Boolean, and one using an invalid string
input (e.g. "name":"x") asserting JTokenType.Null; keep the test helper
RenderValue and the method name unchanged so the test targets the same code
paths.
In `@SW.Bitween.UnitTests/ScribanJsonHelperRootMappingTests.cs`:
- Around line 131-138: The test
PartnerAndGlobalTypedTargets_AreRepresentedAsNullTemplateLiteral currently
hardcodes null in the template so it never exercises __partner__/__globals__
mapping; update the template string (the variable named template) to use typed
conversion/parity expressions that reference the input's __partner__ and
__globals__ properties (e.g., convert __partner__.flag to a typed template
expression and __globals__.S.K to a typed numeric expression) so
Render(template, input) actually triggers the partner/global mapping logic, then
keep the AssertJsonEquals against the expected JSON produced by those typed
conversions; use the Render method and AssertJsonEquals as-is to validate the
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76d64941-a6c6-4686-808c-11f6326118e3
📒 Files selected for processing (9)
SW.Bitween.Api/Resources/Mappers/Preview.csSW.Bitween.UnitTests/NativeJsonMapperTests.csSW.Bitween.UnitTests/RunMapperEnrichmentTests.csSW.Bitween.UnitTests/ScribanGeneratorParityTests.csSW.Bitween.UnitTests/ScribanJsonHelperArrayMappingTests.csSW.Bitween.UnitTests/ScribanJsonHelperErrorHandlingTests.csSW.Bitween.UnitTests/ScribanJsonHelperLookupAndTypeRuleTests.csSW.Bitween.UnitTests/ScribanJsonHelperRootMappingTests.csSW.Bitween.UnitTests/ScribanJsonTestHelper.cs
💤 Files with no reviewable changes (1)
- SW.Bitween.Api/Resources/Mappers/Preview.cs
AhmadRAbuhussein
approved these changes
Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…or Scriban JSON mapping
Summary by CodeRabbit
Bug Fixes
Tests