Bug fix/dev time resetting to zero on reload#3793
Conversation
WalkthroughThe recent updates mainly enhance the handling of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Ginger/GingerCoreCommon/Repository/BusinessFlowLib/Activity.cs (2 hunks)
- Ginger/GingerCoreCommon/Repository/BusinessFlowLib/BusinessFlow.cs (2 hunks)
- Ginger/GingerCoreCommon/Repository/NewRepositorySerializer.cs (2 hunks)
- Ginger/GingerCoreNETUnitTest/SolutionTestsLib/RepositorySerializerTest.cs (2 hunks)
Additional comments not posted (5)
Ginger/GingerCoreNETUnitTest/SolutionTestsLib/RepositorySerializerTest.cs (1)
35-35: Approved: Addition ofSystem.Text.RegularExpressionsnamespace.This is necessary for the new regular expression logic in the test methods.
Ginger/GingerCoreCommon/Repository/BusinessFlowLib/Activity.cs (2)
159-165: Optimization in DevelopmentTime property update logic is approved.The modification to only update the
DevelopmentTimewhen the new value differs from the current value is a good practice. It prevents unnecessary updates and potential performance issues.
191-191: Proper use of the DevelopmentTime property setter in StopTimer method.Using the property setter instead of directly modifying the field ensures that all conditions and logic defined in the setter are respected. This change is crucial for maintaining the integrity of the
DevelopmentTimeupdates.Ginger/GingerCoreCommon/Repository/BusinessFlowLib/BusinessFlow.cs (2)
129-135: Optimization inDevelopmentTimeproperty setter approved.The change to only update
mDevelopmentTimewhen the value actually changes is a good practice to prevent unnecessary operations. This should enhance performance, especially in scenarios where this property might be set frequently.
161-161: Use of property setter inStopTimermethod approved.Using the property setter within the
StopTimermethod ensures that any logic encapsulated in the setter, such as the check for value changes, is consistently applied. This change helps maintain the robustness of the property's behavior.
| else if (propertyInfo.PropertyType == typeof(System.TimeSpan)) | ||
| { | ||
| if (sValue != "00:00:00") | ||
| { | ||
| TimeSpan timeSpan; | ||
| if(TimeSpan.TryParse(sValue, out timeSpan)) | ||
| { | ||
| propertyInfo.SetValue(obj, timeSpan); | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR,"Failed to set the DevelopmentTime"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhanced handling of TimeSpan properties
The modification to handle TimeSpan properties more robustly is a good addition, especially in the context of the bug this PR addresses. However, the error logging could be more descriptive to aid in debugging.
- Reporter.ToLog(eLogLevel.ERROR,"Failed to set the DevelopmentTime");
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to parse TimeSpan from value '{sValue}' for property '{propertyInfo.Name}'");This change will provide more context in the logs, making it easier to identify where and why the failure occurred.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (propertyInfo.PropertyType == typeof(System.TimeSpan)) | |
| { | |
| if (sValue != "00:00:00") | |
| { | |
| TimeSpan timeSpan; | |
| if(TimeSpan.TryParse(sValue, out timeSpan)) | |
| { | |
| propertyInfo.SetValue(obj, timeSpan); | |
| } | |
| else | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR,"Failed to set the DevelopmentTime"); | |
| } | |
| } | |
| else | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Failed to parse TimeSpan from value '{sValue}' for property '{propertyInfo.Name}'"); | |
| } |
| //Assert.IsTrue(xml.Contains("<BusinessFlow Guid=")); | ||
| Regex regex = new(@"<BusinessFlow.*\sGuid="); // added for handling the development time tag in between of guid | ||
| Assert.IsTrue(regex.Matches(xml).Count>0); |
There was a problem hiding this comment.
Refactor Suggestion: Optimize Regular Expression Usage.
The regular expression new Regex(@"<BusinessFlow.*\sGuid=") is used to match XML tags more flexibly. However, this approach might be inefficient due to the broad pattern used. Consider refining the regex pattern to be more specific or using direct string methods if the structure of the XML is well-known and consistent.
- Regex regex = new(@"<BusinessFlow.*\sGuid=");
+ Regex regex = new(@"<BusinessFlow[^>]*Guid=");This change makes the regular expression less prone to excessive backtracking and improves performance by ensuring it does not match across multiple lines unintentionally.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Assert.IsTrue(xml.Contains("<BusinessFlow Guid=")); | |
| Regex regex = new(@"<BusinessFlow.*\sGuid="); // added for handling the development time tag in between of guid | |
| Assert.IsTrue(regex.Matches(xml).Count>0); | |
| //Assert.IsTrue(xml.Contains("<BusinessFlow Guid=")); | |
| Regex regex = new(@"<BusinessFlow[^>]*Guid="); // added for handling the development time tag in between of guid | |
| Assert.IsTrue(regex.Matches(xml).Count>0); |
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
Bug Fixes
DevelopmentTimeproperty updates to avoid unnecessary assignments inActivityandBusinessFlow.New Features
TimeSpanproperty settings.Tests