Fetaure/version mismatch#4288
Conversation
1. Updated status enum with inrpgoress status to match with account report contract 2. Removed duplicate data being sent to account report 3. Added new way to comapre versions using records and also added its unit test cases
|
Caution Review failedThe pull request is closed. WalkthroughAdds eRunStatus.InProgress; introduces VersionParts record and ConvertApplicationVersionToRecord; updates AccountReport end-data mappings to set StartTimeStamp and stop populating certain identity fields; implements record-based solution-version comparison with new APIs and tests; updates solution upgrade flow to use record-based comparison. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant SU as SolutionUpgrade
participant FS as FileSystem
participant AI as ApplicationInfo
rect rgba(221,238,255,0.5)
note over Caller,SU: New record-based comparison flow
Caller->>SU: CompareSolutionFileGingerVersionToCurrentByRecord(filePath, ref fileVerStr)
SU->>FS: Read file and extract GingerVersion
FS-->>SU: fileVerStr
SU->>AI: ConvertApplicationVersionToRecord(ApplicationBackendVersion)
AI-->>SU: appVersion (VersionParts)
SU->>AI: ConvertApplicationVersionToRecord(fileVerStr)
AI-->>SU: fileVersion (VersionParts)
end
alt fileVersion == "3.0.0.0Beta"
SU->>SU: treat as LowerVersion (special-case)
end
rect rgba(232,245,233,0.5)
SU->>SU: CompareVersionsByRecord(appVersion, fileVersion)
SU-->>Caller: eGingerVersionComparisonResult (Lower / Same / Higher / ComparisonFailed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreCommon/GeneralLib/ApplicationInfo.cs (1)
175-225: Keep semantics aligned between ConvertApplicationVersionToLong and …ToRecord.Both default 2-part versions to Build=99, Revision=99; good. Consider adding a single internal helper to avoid drift in future edits.
I can extract a shared parser returning VersionParts and rewrite ConvertApplicationVersionToLong to consume it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Ginger/GingerCoreCommon/EnumsLib/eStatus.cs(1 hunks)Ginger/GingerCoreCommon/GeneralLib/ApplicationInfo.cs(4 hunks)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportEntitiesDataMapping.cs(6 hunks)Ginger/GingerCoreNET/SolutionRepositoryLib/UpgradeLib/SolutionUpgrade.cs(3 hunks)Ginger/GingerCoreNETUnitTest/SolutionRepositoryLib/SolutionUpgradeTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Ginger/GingerCoreCommon/GeneralLib/ApplicationInfo.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/GingerCoreNETUnitTest/SolutionRepositoryLib/SolutionUpgradeTest.cs (2)
Ginger/GingerCoreNET/SolutionRepositoryLib/UpgradeLib/SolutionUpgrade.cs (1)
SolutionUpgrade(51-478)Ginger/GingerCoreCommon/GeneralLib/ApplicationInfo.cs (1)
VersionParts(227-263)
Ginger/GingerCoreNET/SolutionRepositoryLib/UpgradeLib/SolutionUpgrade.cs (2)
Ginger/GingerCoreCommon/GeneralLib/ApplicationInfo.cs (3)
ApplicationInfo(25-264)ApplicationInfo(29-31)VersionParts(227-263)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (11)
Ginger/GingerCoreNET/SolutionRepositoryLib/UpgradeLib/SolutionUpgrade.cs (1)
29-29: Good use of static imports for cleaner code.The
using staticdirective improves readability by avoiding repetitive namespace qualifications.Ginger/GingerCoreNETUnitTest/SolutionRepositoryLib/SolutionUpgradeTest.cs (3)
46-52: Good test coverage for enum values.The test ensures all expected enum values are defined, which helps catch accidental removals or renames.
55-107: Excellent test coverage for version comparison scenarios.The tests comprehensively cover various scenarios (lower, same, higher, invalid, beta versions) and verify consistency between the old and new comparison methods. This ensures backward compatibility.
80-80: Nice touch with the future-proof comment!The comment "For People who will be using Ginger in 2050" adds humor while clearly indicating the test's purpose.
Ginger/GingerCoreCommon/EnumsLib/eStatus.cs (1)
37-39: Ensure numeric parity between eRunStatus and external eExecutionStatuseRunStatus relies on implicit sequential values, while eExecutionStatus lives in AccountReport.Contracts.Enum (not in this repo) and is cast directly—please verify its numeric values match or introduce explicit assignments or an explicit mapping function to prevent invalid casts.
Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportEntitiesDataMapping.cs (6)
87-92: EndData: Identity fields removed — confirm API contract for updates.By omitting EntityId/AccountReportDbActivityId/ExecutionId/Name in EndData, the payload relies on Id alone for upsert/correlation. Validate the centralized report API doesn’t require these fields on “end” updates.
If there’s an API schema file, please point to it; otherwise I can grep for the DTO validation on the server side.
166-171: Activity EndData: same identity-field removal — verify backend expectations.Same concern as for actions. If the server uses composite keys, this change could break deduplication.
I can scan for server-side usage of these fields if you share the service repo path.
246-251: ActivityGroup EndData: identity fields removed — verify.Repeat of the above for activity groups. Ensure “Start then End” flow sends enough identifiers for an idempotent update.
Happy to help add integration tests for the ingestion path.
317-317: StartTimeStamp added to BusinessFlow EndData — LGTM.Good for parity with StartData; avoids null StartTimeStamp server-side.
449-449: Runner EndData StartTimeStamp — LGTM.Consistent with other entities; no issues spotted.
534-534: RunSet EndData StartTimeStamp — LGTM.Matches the StartData/EndData symmetry across entities.
1 participant
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Bug Fixes
Tests