Runset variable customization simplification#4330
Merged
Maheshkale447 merged 21 commits intoNov 12, 2025
Merged
Conversation
…rent repo wont build
"Value" that allows the user to change the current value of a variable, this function now works for string variables
…, currently working on date variables
…ports date time variables.
…umn is now hidden aswell
…able_customization_simplification
Contributor
WalkthroughAdds a new Value data-mapping mode across UI, variable model, deserialization, execution, and analysis; migrates legacy mappings to Value-based format; updates variable editing and add flows to respect SupportSetValue changes and exposes CurrentEffectiveValue/InitialValue properties. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UC as UCDataMapping
participant Valid as Validation
participant VB as VariableBase
participant Exec as ExecutionEngine
User->>UC: set MappedType = Value
UC->>UC: show Value input UI (selection / date / text)
User->>UC: enter/select value
UC->>Valid: validate input (numeric/date/etc.)
Valid-->>UC: ValidationResult (IsValid/Error)
alt valid
UC->>VB: set MappedOutputType = Value
UC->>VB: set MappedOutputValue = user value
VB-->>Exec: expose CurrentEffectiveValue
Exec->>Exec: apply mapped value to inputs/outputs
else invalid
UC-->>User: show error tooltip / red border
end
sequenceDiagram
participant Loader as Deserializer
participant VB as VariableBase
participant Compat as CompatibilityLogic
Loader->>VB: load legacy mapping (DifferentFromOrigin=true, MappedOutputType=None)
VB->>Compat: PostDeserialization / RunsetExecutor migration invoked
Compat->>Compat: GetVariableSourceValue(var) -> source
Compat->>VB: set MappedOutputType = Value, MappedOutputValue = source
VB-->>Loader: variable migrated to Value mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention areas:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
…n with mapped output type value
noahdegen
marked this pull request as ready for review
October 21, 2025 07:15
…tomization_simplification
…//github.com/Ginger-Automation/Ginger into runset_variable_customization_simplification
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs (1)
700-709: EnableDataMapping=false is ignored; control stays enabledSetter forces IsEnabled=true but never disables on false. This can expose inactive mapping UI.
Apply this diff:
- private void EnableDataMappingPropertyChanged(bool enabelMapping) + private void EnableDataMappingPropertyChanged(bool enabelMapping) { EnableDataMapping = enabelMapping; if (EnableDataMapping == true) { - this.IsEnabled = true; + this.IsEnabled = true; // When data mapping is enabled, ensure fundamental options are available GingerCore.General.EnableComboItem(xMappedTypeComboBox, eDataType.None); GingerCore.General.EnableComboItem(xMappedTypeComboBox, eDataType.Value); } + else + { + this.IsEnabled = false; + } }Ginger/GingerCoreCommon/VariablesLib/VariableDynamic.cs (1)
117-132: Contract violation confirmed: SetValue must respect SupportSetValueThe base class
VariableBase.SetValue()explicitly checksif (this.SupportSetValue)before mutating state and returns false if unsupported.VariableDynamicbreaks this contract by unconditionally settingValueExpressionand returning true despiteSupportSetValuereturning false. This violates the established pattern: callers inGingerExecutionEngine.csand UI components guard SetValue calls with SupportSetValue checks, and other variable types align their SupportSetValue return value with their SetValue behavior.Apply the suggested fix to restore consistency:
public override bool SetValue(string value) { - ValueExpression = value; - return true; + // Not supported for dynamic variable + return false; }Alternatively, if setting ValueExpression is intended, change
SupportSetValueto return true and document the behavior.Ginger/Ginger/Variables/VariableEditPage.xaml.cs (1)
435-446: Publish uses raw Value; should use effective valueWhen mapping type is Value, publishing mVariable.Value may push the wrong content. Use CurrentEffectiveValue.
- Value = mVariable.Value, + Value = mVariable.CurrentEffectiveValue,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
Ginger/Ginger/Run/BusinessFlowRunConfigurationsPage.xaml.cs(3 hunks)Ginger/Ginger/UserControlsLib/UCDataMapping.xaml(2 hunks)Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs(14 hunks)Ginger/Ginger/Variables/AddVariablePage.xaml.cs(2 hunks)Ginger/Ginger/Variables/VariableEditPage.xaml(1 hunks)Ginger/Ginger/Variables/VariableEditPage.xaml.cs(5 hunks)Ginger/GingerCore/GeneralLib/GenericWindow.xaml(1 hunks)Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs(4 hunks)Ginger/GingerCoreCommon/VariablesLib/VariableDynamic.cs(1 hunks)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs(2 hunks)Ginger/GingerCoreNET/AnalyzerLib/RunSetConfigAnalyzer.cs(1 hunks)Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs(1 hunks)Ginger/GingerCoreNET/Run/RunsetExecutor.cs(3 hunks)Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T13:44:51.210Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4245
File: Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs:658-667
Timestamp: 2025-07-09T13:44:51.210Z
Learning: In UCDataMapping.xaml.cs SetDatabaseValues() method, the line xDBValueExpression.Visibility = Visibility.Visible is intentionally set unconditionally and works correctly with the visibility logic in SetValueControlsView(). This is the expected behavior per user confirmation.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
🧬 Code graph analysis (8)
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (1)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
GetInitialValue(834-837)
Ginger/GingerCoreNET/AnalyzerLib/RunSetConfigAnalyzer.cs (1)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)
Ginger/GingerCoreNET/Run/RunsetExecutor.cs (5)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (3)
VariableBase(39-870)GetInitialValue(834-837)SetInitialValue(323-323)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (4)
VariablePasswordString(25-106)VariablePasswordString(27-29)GetInitialValue(98-101)SetInitialValue(86-96)Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs (1)
VariableNumber(26-348)Ginger/GingerCoreCommon/VariablesLib/VariableDateTime.cs (1)
VariableDateTime(26-262)
Ginger/Ginger/Variables/VariableEditPage.xaml.cs (2)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserElementLocator.cs (1)
Context(40-46)
Ginger/Ginger/Run/BusinessFlowRunConfigurationsPage.xaml.cs (1)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs (5)
Ginger/GingerCore/GeneralLib/General.cs (3)
EnableComboItem(818-831)DisableComboItem(782-795)CheckComboItemExist(384-405)Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (3)
ToString(69-72)VariableBase(39-870)SetValue(762-773)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreCommon/VariablesLib/VariableDateTime.cs (2)
VariableDateTime(26-262)CheckDateTimeWithInRange(222-237)Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs (1)
VariableNumber(26-348)
Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (2)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (5)
Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs (5)
ValueExpression(47-2065)ValueExpression(183-192)ValueExpression(193-202)ValueExpression(208-212)ValueExpression(213-213)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (4)
GetInitialValue(98-101)ResetValue(62-65)VariablePasswordString(25-106)VariablePasswordString(27-29)Ginger/GingerCoreCommon/VariablesLib/VariableDateTime.cs (3)
GetInitialValue(258-261)ResetValue(208-211)VariableDateTime(26-262)Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs (3)
GetInitialValue(344-347)ResetValue(319-322)VariableNumber(26-348)Ginger/GingerCoreCommon/VariablesLib/VariableString.cs (4)
GetInitialValue(108-111)ResetValue(68-71)VariableString(25-112)VariableString(27-29)
🔇 Additional comments (17)
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs (2)
426-465: Value mapping removes DateTime constraints entirely; confirm requirementMultiple places allow “ALL dates” in Value mode, bypassing VariableDateTime.Min/Max. If that’s not the intended product behavior, reapply constraints or make it configurable.
Also applies to: 880-989, 1057-1090
1121-1129: xDBValueExpression visibility contractPer prior learnings, SetDatabaseValues() intentionally sets xDBValueExpression.Visibility = Visible unconditionally and relies on SetValueControlsView() to reconcile visibility. Ensure that behavior is preserved after these changes. Based on learnings
Ginger/GingerCore/GeneralLib/GenericWindow.xaml (1)
58-60: Verify whether this whitespace change is intentionalThe review comment is accurate. The git diff confirms the only change on line 59 is adding a space before the closing
>:Background="Transparent">becomesBackground="Transparent" >.Since no formatter configuration enforces this style and the change appears isolated rather than part of systematic reformatting across the codebase, the recommendation to revert is sound unless this change is deliberate. You should verify: (1) whether this formatting style is being standardized in the PR or codebase, and (2) whether the change should be reverted to reduce unnecessary churn.
Ginger/GingerCoreNET/AnalyzerLib/RunSetConfigAnalyzer.cs (1)
161-165: Apply mandatory input validation for Value-type mappingsThe proposed fix is sound. All required properties exist on VariableBase:
Value(line 160): public virtual string propertyMandatoryInput(line ~96): public bool propertyMappedOutputValue(line 737): public string propertyThe current code incorrectly treats Value-type inputs as always valid, bypassing mandatory input checks. The proposed fix correctly validates that when
MandatoryInputis true and bothMappedOutputValueandValueare empty, an issue should be flagged—consistent with validation patterns in other output type cases (e.g., DataSource validatesMappedOutputValuepresence).Apply the diff as proposed.
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (1)
98-101: InitialValue binding OK; confirm UI masks secrets.GetInitialValue returns the encrypted Password. Ensure UI templates mask this (stars) to avoid showing ciphertext strings.
Based on learnings.
Ginger/Ginger/Variables/AddVariablePage.xaml.cs (1)
369-381: Good: Input flags gated by SupportSetValue.Prevents invalid input configuration for non-settable variables.
Ginger/Ginger/Run/BusinessFlowRunConfigurationsPage.xaml.cs (3)
91-93: Disabling double‑click edit: confirm alt access remains.Since RowDoubleClick is removed, ensure btnEdit and keyboard affordances remain for accessibility.
167-167: Using InitialValue in grid aligns with new model.Column switch to InitialValue matches VariableBase.GetInitialValue usage.
184-184: Mapping source bound to PossibleOutputVariables looks correct.Template now uses the output variables list; consistent with LoadGridData setup.
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (1)
750-753: Value mapping case wired correctlySwitch case maps VariableBase.eOutputType.Value to InputValue.eVariableCustomizationType.Value and assigns MappedOutputValue. Looks good.
Ginger/Ginger/Variables/VariableEditPage.xaml.cs (1)
462-466: Expander preference tracking is fineMarking HasUserSetPreference on expand/collapse is appropriate.
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml (1)
21-31: General: tooltip binding pattern and styles look consistentNo issues spotted with the mapping-type and variable combobox scaffolding.
Also applies to: 33-41
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (4)
83-85: Enum extension adds Value at the end — good for binary compatibilityAppending Value avoids shifting existing underlying values.
159-161: Notify CurrentEffectiveValue on Value change — goodKeeps bound UIs in sync when base Value changes.
191-216: Legacy-to-Value migration is fine; double-check ResetValue side-effectAfter converting to Value mapping, calling ResetValue restores original Value, which is OK since the effective display comes from mapping; just ensure run-time code applies mapping when needed (see RunsetExecutor).
719-733: MappedOutputType/Value raise CurrentEffectiveValue — goodChange notifications are complete.
Also applies to: 736-751
Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
269-284: Legacy → Value migration at load is soundEarly conversion reduces downstream branching.
Comment on lines
+571
to
+614
| private void xValueTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) | ||
| { | ||
| // Check if this is a number variable context based on the DataContext | ||
| if (DataContext is VariableNumber) | ||
| { | ||
| // Allow only valid numeric input for number variables | ||
| TextBox textBox = (TextBox)sender; | ||
| e.Handled = !IsValidNumericInput(e.Text, textBox); | ||
| } | ||
| } | ||
|
|
||
| private bool IsValidNumericInput(string inputText, TextBox textBox) | ||
| { | ||
| // Get the current text and construct what the new text would be | ||
| string currentText = textBox.Text ?? ""; | ||
| int selectionStart = textBox.SelectionStart; | ||
| int selectionLength = textBox.SelectionLength; | ||
|
|
||
| // Construct the new text after the input | ||
| string newText = currentText.Remove(selectionStart, selectionLength).Insert(selectionStart, inputText); | ||
|
|
||
| // Allow empty string (user can delete everything) | ||
| if (string.IsNullOrEmpty(newText)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Allow single minus sign at the beginning for negative numbers | ||
| if (newText == "-") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Allow single decimal point | ||
| if (newText == ".") | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Use regex to check for valid numeric input (including decimal numbers) | ||
| // This allows: optional minus, digits, optional decimal point with digits | ||
| var numericRegex = new Regex(@"^-?(\d+\.?\d*|\.\d+)$"); | ||
| return numericRegex.IsMatch(newText); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Numeric input validation ignores locale and integer-only variables
Current logic hardcodes '.' and allows decimals even for integer variables, causing false “valid” UI while VariableNumber.SetValue later rejects. Also parsing uses InvariantCulture, diverging from user locale.
Apply this diff to respect CurrentCulture and integer-only:
- private void xValueTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e)
+ private void xValueTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e)
{
// Check if this is a number variable context based on the DataContext
- if (DataContext is VariableNumber)
+ if (DataContext is VariableNumber numberVar)
{
// Allow only valid numeric input for number variables
TextBox textBox = (TextBox)sender;
- e.Handled = !IsValidNumericInput(e.Text, textBox);
+ e.Handled = !IsValidNumericInput(e.Text, textBox, numberVar);
}
}
- private bool IsValidNumericInput(string inputText, TextBox textBox)
+ private bool IsValidNumericInput(string inputText, TextBox textBox, VariableNumber numberVar)
{
// Get the current text and construct what the new text would be
string currentText = textBox.Text ?? "";
int selectionStart = textBox.SelectionStart;
int selectionLength = textBox.SelectionLength;
// Construct the new text after the input
string newText = currentText.Remove(selectionStart, selectionLength).Insert(selectionStart, inputText);
// Allow empty string (user can delete everything)
if (string.IsNullOrEmpty(newText))
{
return true;
}
-
- // Allow single minus sign at the beginning for negative numbers
+
+ var nfi = System.Globalization.CultureInfo.CurrentCulture.NumberFormat;
+ string dec = System.Text.RegularExpressions.Regex.Escape(nfi.NumberDecimalSeparator);
+
+ // Allow single minus sign at the beginning for negative numbers
if (newText == "-")
{
return true;
}
-
- // Allow single decimal point
- if (newText == ".")
+
+ // Allow single decimal separator by itself
+ if (newText == nfi.NumberDecimalSeparator)
{
- return true;
+ // Only if decimals are allowed
+ return numberVar.IsDecimalValue;
}
-
- // Use regex to check for valid numeric input (including decimal numbers)
- // This allows: optional minus, digits, optional decimal point with digits
- var numericRegex = new Regex(@"^-?(\d+\.?\d*|\.\d+)$");
- return numericRegex.IsMatch(newText);
+
+ // Build regex: integer vs decimal (locale-aware)
+ string integerPattern = @"^-?\d+$";
+ string decimalPattern = $@"^-?(\d+{dec}?\d*|{dec}\d+)$";
+ var pattern = numberVar.IsDecimalValue ? decimalPattern : integerPattern;
+ return new Regex(pattern).IsMatch(newText);
}
- private ValidationResult ValidateNumericValue(string value)
+ private ValidationResult ValidateNumericValue(string value)
{
- if (!double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double numericValue))
+ var nfi = CultureInfo.CurrentCulture;
+ if (DataContext is VariableNumber numberVar)
+ {
+ // Enforce integer-only when required
+ if (!numberVar.IsDecimalValue && value.Contains(nfi.NumberDecimalSeparator))
+ {
+ return new ValidationResult(false, "Integer value expected");
+ }
+ }
+
+ if (!double.TryParse(value, NumberStyles.Float, nfi, out double numericValue))
{
return new ValidationResult(false, "Invalid numeric format");
}
return new ValidationResult(true);
}Also applies to: 638-646
I'll verify the claims in this review comment by examining the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find VariableNumber class definition
rg -nP 'class\s+VariableNumber' --type=cs -A 15
Length of output: 3221
🏁 Script executed:
#!/bin/bash
# Find the ValidateNumericValue method (lines around 638-646)
rg -nP 'ValidateNumericValue' --type=cs -B 2 -A 10Length of output: 1727
🏁 Script executed:
#!/bin/bash
# Search for IsDecimalValue property in the codebase
rg -nP 'IsDecimalValue' --type=cs -B 2 -A 2Length of output: 7907
🏁 Script executed:
#!/bin/bash
# Search for InvariantCulture usage in the file
rg -nP 'InvariantCulture' --type=cs -B 3 -A 3Length of output: 61683
🏁 Script executed:
#!/bin/bash
# Check the UCDataMapping.xaml.cs file for context around the methods
rg -nP 'xValueTextBox_PreviewTextInput|IsValidNumericInput' --type=cs -B 3 -A 15Length of output: 8504
I need to see the complete IsValidNumericInput method to verify all claims. Let me search further:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete IsValidNumericInput method (starting from line 582)
sed -n '582,620p' Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
Length of output: 1561
🏁 Script executed:
#!/bin/bash
# Search for how VariableNumber handles SetValue to confirm it rejects decimals for integer types
rg -nP 'public.*SetValue|if.*IsDecimalValue' Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs -A 5 -B 2Length of output: 2414
🏁 Script executed:
#!/bin/bash
# Check if there are any SetValue/validation methods that handle decimal vs integer
rg -nP 'IsIntegerValue|IsDecimalValue' Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs -B 3 -A 8Length of output: 3279
Confirm review comment addresses genuine validation mismatch
The review identifies real critical issues verified by codebase inspection:
-
Integer-validation bug: IsValidNumericInput allows decimals unconditionally with hardcoded "." separator (line 607), but VariableNumber.SetValue rejects decimals when IsDecimalValue=false (line 301). This creates false "valid" UI where users can type values that are immediately rejected.
-
Locale issue: The hardcoded "." ignores the user's locale decimal separator, and ValidateNumericValue uses InvariantCulture instead of respecting the current culture.
The proposed diff correctly:
- Passes the VariableNumber instance to IsValidNumericInput to check IsDecimalValue
- Uses CultureInfo.CurrentCulture.NumberFormat for locale-aware separator detection
- Enforces integer-only validation matching SetValue's behavior
- Updates ValidateNumericValue to use CurrentCulture for parsing
🤖 Prompt for AI Agents
In Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs around lines 571 to 614,
the input validation routine allows decimals and uses a hardcoded "." and
InvariantCulture which conflicts with VariableNumber.SetValue (which may
disallow decimals) and ignores locale; update the handlers and validators to
accept the VariableNumber instance (pass DataContext as VariableNumber into
IsValidNumericInput), check its IsDecimalValue flag and reject decimal input
when false, derive the decimal separator from
CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator instead of "."
(and adjust the validation logic/regex accordingly), and change
ValidateNumericValue parsing to use CurrentCulture so UI validation behavior
matches SetValue and respects user locale.
Comment on lines
+123
to
+139
| // Check if the selected variable supports set value operation | ||
| if (SelectedListView is VariableBase selectedVariable) | ||
| { | ||
| // Hide "Set as Input" checkbox for variables that don't support setting values | ||
| if (!selectedVariable.SupportSetValue) | ||
| { | ||
| xSetAsInputValueCheckBox.Visibility = Visibility.Collapsed; | ||
| xMandatoryInputCheckBox.Visibility = Visibility.Collapsed; | ||
| // Keep other checkboxes visible since output doesn't require SupportSetValue | ||
| } | ||
| else | ||
| { | ||
| // Show "Set as Input" checkbox for variables that support setting values | ||
| xSetAsInputValueCheckBox.Visibility = Visibility.Visible; | ||
| // Mandatory input checkbox visibility is controlled by the InputOutputChecked event | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stabilize checkbox state on selection change.
When switching to a non-settable variable, clear prior selections; when settable, set Mandatory visibility per current Input checkbox state.
@@
- if (SelectedListView is VariableBase selectedVariable)
+ if (SelectedListView is VariableBase selectedVariable)
{
- // Hide "Set as Input" checkbox for variables that don't support setting values
- if (!selectedVariable.SupportSetValue)
+ // Hide/clear input-related checkboxes for variables that don't support setting values
+ if (!selectedVariable.SupportSetValue)
{
+ xSetAsInputValueCheckBox.IsChecked = false;
+ xMandatoryInputCheckBox.IsChecked = false;
xSetAsInputValueCheckBox.Visibility = Visibility.Collapsed;
xMandatoryInputCheckBox.Visibility = Visibility.Collapsed;
// Keep other checkboxes visible since output doesn't require SupportSetValue
}
else
{
// Show "Set as Input" checkbox for variables that support setting values
xSetAsInputValueCheckBox.Visibility = Visibility.Visible;
- // Mandatory input checkbox visibility is controlled by the InputOutputChecked event
+ // Set Mandatory visibility based on current state to avoid stale UI
+ xMandatoryInputCheckBox.Visibility = xSetAsInputValueCheckBox.IsChecked == true ? Visibility.Visible : Visibility.Collapsed;
}
}📝 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.
Suggested change
| // Check if the selected variable supports set value operation | |
| if (SelectedListView is VariableBase selectedVariable) | |
| { | |
| // Hide "Set as Input" checkbox for variables that don't support setting values | |
| if (!selectedVariable.SupportSetValue) | |
| { | |
| xSetAsInputValueCheckBox.Visibility = Visibility.Collapsed; | |
| xMandatoryInputCheckBox.Visibility = Visibility.Collapsed; | |
| // Keep other checkboxes visible since output doesn't require SupportSetValue | |
| } | |
| else | |
| { | |
| // Show "Set as Input" checkbox for variables that support setting values | |
| xSetAsInputValueCheckBox.Visibility = Visibility.Visible; | |
| // Mandatory input checkbox visibility is controlled by the InputOutputChecked event | |
| } | |
| } | |
| // Check if the selected variable supports set value operation | |
| if (SelectedListView is VariableBase selectedVariable) | |
| { | |
| // Hide/clear input-related checkboxes for variables that don't support setting values | |
| if (!selectedVariable.SupportSetValue) | |
| { | |
| xSetAsInputValueCheckBox.IsChecked = false; | |
| xMandatoryInputCheckBox.IsChecked = false; | |
| xSetAsInputValueCheckBox.Visibility = Visibility.Collapsed; | |
| xMandatoryInputCheckBox.Visibility = Visibility.Collapsed; | |
| // Keep other checkboxes visible since output doesn't require SupportSetValue | |
| } | |
| else | |
| { | |
| // Show "Set as Input" checkbox for variables that support setting values | |
| xSetAsInputValueCheckBox.Visibility = Visibility.Visible; | |
| // Set Mandatory visibility based on current state to avoid stale UI | |
| xMandatoryInputCheckBox.Visibility = xSetAsInputValueCheckBox.IsChecked == true ? Visibility.Visible : Visibility.Collapsed; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Ginger/Ginger/Variables/AddVariablePage.xaml.cs around lines 123 to 139, the
checkbox state isn’t reset when switching to a variable that does not support
setting values and the Mandatory checkbox visibility isn’t synchronized when
switching back to a settable variable; update the selection handling so that
when selectedVariable.SupportSetValue is false you not only collapse
xSetAsInputValueCheckBox and xMandatoryInputCheckBox but also clear their
IsChecked state (set to false) and any related flags, and when SupportSetValue
is true ensure xMandatoryInputCheckBox.Visibility is set based on the current
xSetAsInputValueCheckBox.IsChecked value (Visible when checked, Collapsed when
unchecked) so the UI reflects the current Input checkbox state immediately on
selection change.
Comment on lines
1113
to
1114
| runSetConfig.ReRunConfigurations.ReferenceExecutionID = gingerExecConfig.Runset.RerunConfigurations.ReferenceExecutionID.Value; | ||
| } |
Contributor
There was a problem hiding this comment.
Null-safety for ReferenceExecutionID.Value
Calling .Value on a nullable Guid can throw if not provided, even when Active is true. Guard with HasValue or use GetValueOrDefault().
Apply:
- runSetConfig.ReRunConfigurations.ReferenceExecutionID = gingerExecConfig.Runset.RerunConfigurations.ReferenceExecutionID.Value;
+ var refId = gingerExecConfig.Runset.RerunConfigurations.ReferenceExecutionID;
+ runSetConfig.ReRunConfigurations.ReferenceExecutionID = refId.HasValue ? refId.Value : Guid.Empty;📝 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.
Suggested change
| runSetConfig.ReRunConfigurations.ReferenceExecutionID = gingerExecConfig.Runset.RerunConfigurations.ReferenceExecutionID.Value; | |
| } | |
| var refId = gingerExecConfig.Runset.RerunConfigurations.ReferenceExecutionID; | |
| runSetConfig.ReRunConfigurations.ReferenceExecutionID = refId.HasValue ? refId.Value : Guid.Empty; | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
around lines 1113-1114, the code dereferences a nullable Guid via .Value which
can throw if the nullable has no value; change the assignment to guard for null
by either checking Runset.RerunConfigurations.ReferenceExecutionID.HasValue
before using .Value and only assign when true, or replace .Value with
.GetValueOrDefault() (and handle the default Guid.Empty case appropriately) so
the code no longer throws on null.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (3)
491-509: Guard against null UserProfile in CLI pathsWorkSpace.Instance.UserProfile may be null; accessing ALMUserConfigs risks NRE. Use null-propagation.
- ALMUserConfig userProfileAlmConfig = WorkSpace.Instance.UserProfile.ALMUserConfigs.FirstOrDefault(x => x.AlmType == solutionAlmConfig.AlmType); + ALMUserConfig userProfileAlmConfig = WorkSpace.Instance.UserProfile?.ALMUserConfigs? + .FirstOrDefault(x => x.AlmType == solutionAlmConfig.AlmType);
1888-1905: Fix NRE in error message when name is nullname.Item2 can be null; calling ToLower() throws and masks the real lookup error.
- string error = string.Format("Failed to find {0} with the details '{1}/{2}'", typeof(T), name.Item2.ToLower(), id.Item2); + var nameValueSafe = name.Item2?.ToLower() ?? string.Empty; + string error = string.Format("Failed to find {0} with the details '{1}/{2}'", typeof(T), nameValueSafe, id.Item2);Apply the same change in the catch block.
1545-1547: Harden SMTP port parsingint.Parse throws on bad input from external JSON. Prefer TryParse with fallback/log.
- mailOperation.Email.SMTPPort = int.Parse(runsetOperationConfigMail.MailSettings.SmtpDetails.Port); + if (int.TryParse(runsetOperationConfigMail.MailSettings.SmtpDetails.Port, out var port)) + mailOperation.Email.SMTPPort = port; + else + Reporter.ToLog(eLogLevel.WARN, $"Invalid SMTP port '{runsetOperationConfigMail.MailSettings.SmtpDetails.Port}'. Using default.");Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
1888-1905: Same NRE risk in lookup error messagename.Item2 may be null. Use a safe string like in the other file.
- string error = string.Format("Failed to find {0} with the details '{1}/{2}'", typeof(T), name.Item2.ToLower(), id.Item2); + var nameValueSafe = name.Item2?.ToLower() ?? string.Empty; + string error = string.Format("Failed to find {0} with the details '{1}/{2}'", typeof(T), nameValueSafe, id.Item2);Also update the catch block.
♻️ Duplicate comments (7)
Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (1)
944-950: Value mapping rejects explicit empty strings; allow clearing variables.Current check skips assignment when MappedOutputValue == "" leading to error handling below. Assign the literal value even when empty for parity with other branches.
Apply:
- else if (inputVar.MappedOutputType == VariableBase.eOutputType.Value) - { - if (!string.IsNullOrEmpty(inputVar.MappedOutputValue)) - { - mappedValue = inputVar.MappedOutputValue; - } - } + else if (inputVar.MappedOutputType == VariableBase.eOutputType.Value) + { + // Literal value mapping (allow explicit empty string) + mappedValue = inputVar.MappedOutputValue ?? string.Empty; + }Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (1)
1503-1511: Safer cast for HTML email operation lookup — goodUsing “as” + null-check avoids InvalidCastException; logging the mismatch is helpful. Thanks for addressing the earlier feedback.
Ginger/GingerCoreNET/Run/RunsetExecutor.cs (2)
307-324: Centralize source-value extraction — nice reuseDelegating to VariableBase.GetVariableSourceValueForDeserialization reduces drift and improves maintainability.
370-394: Correct order: restore initial before applying Value mappingApplying SetInitialValue first and then setting the mapped Value avoids clobbering the mapped value; password encryption handling is correct.
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
162-178: Prevent serialization of computed CurrentEffectiveValueMark as JsonIgnore to avoid leaking a UI-only/computed value into payloads.
- public string CurrentEffectiveValue + [Newtonsoft.Json.JsonIgnore] + public string CurrentEffectiveValue { get { if (MappedOutputType == eOutputType.Value && !string.IsNullOrEmpty(MappedOutputValue)) return MappedOutputValue; return Value; } }Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs (2)
571-580: Numeric validation ignores locale and integer-only; align with VariableNumberUse CurrentCulture’s decimal separator and respect IsDecimalValue to prevent UI accepting values that SetValue later rejects.
- private void xValueTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) + private void xValueTextBox_PreviewTextInput(object sender, TextCompositionEventArgs e) { // Check if this is a number variable context based on the DataContext - if (DataContext is VariableNumber) + if (DataContext is VariableNumber numberVar) { // Allow only valid numeric input for number variables TextBox textBox = (TextBox)sender; - e.Handled = !IsValidNumericInput(e.Text, textBox); + e.Handled = !IsValidNumericInput(e.Text, textBox, numberVar); } } - private bool IsValidNumericInput(string inputText, TextBox textBox) + private bool IsValidNumericInput(string inputText, TextBox textBox, VariableNumber numberVar) { // Get the current text and construct what the new text would be string currentText = textBox.Text ?? ""; int selectionStart = textBox.SelectionStart; int selectionLength = textBox.SelectionLength; // Construct the new text after the input string newText = currentText.Remove(selectionStart, selectionLength).Insert(selectionStart, inputText); // Allow empty string (user can delete everything) if (string.IsNullOrEmpty(newText)) { return true; } // Allow single minus sign at the beginning for negative numbers if (newText == "-") { return true; } - - // Allow single decimal point - if (newText == ".") - { - return true; - } - - // Use regex to check for valid numeric input (including decimal numbers) - // This allows: optional minus, digits, optional decimal point with digits - var numericRegex = new Regex(@"^-?(\d+\.?\d*|\.\d+)$"); - return numericRegex.IsMatch(newText); + + var nfi = CultureInfo.CurrentCulture.NumberFormat; + string dec = Regex.Escape(nfi.NumberDecimalSeparator); + + // Allow decimal separator alone only when decimals are allowed + if (newText == nfi.NumberDecimalSeparator) + { + return numberVar.IsDecimalValue; + } + + // Locale-aware pattern; integer vs decimal + string integerPattern = @"^-?\d+$"; + string decimalPattern = $@"^-?(\d+{dec}?\d*|{dec}\d+)$"; + var pattern = numberVar.IsDecimalValue ? decimalPattern : integerPattern; + return Regex.IsMatch(newText, pattern); } - private ValueValidationResult ValidateNumericValue(string value) + private ValueValidationResult ValidateNumericValue(string value) { - if (!double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out double numericValue)) + var nfi = CultureInfo.CurrentCulture; + if (DataContext is VariableNumber numberVar) + { + if (!numberVar.IsDecimalValue && value.Contains(nfi.NumberDecimalSeparator)) + { + return new ValueValidationResult(false, "Integer value expected"); + } + } + if (!double.TryParse(value, NumberStyles.Float, nfi, out _)) { return new ValueValidationResult(false, "Invalid numeric format"); } return new ValueValidationResult(true); }Also applies to: 582-614, 638-646
848-861: Selection-list parsing ignores newline-separated entriesSupport pasted lists with newlines, not just commas.
- string[] values = formula.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries); + string[] values = formula.Split(new[] { ',', '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml(2 hunks)Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs(14 hunks)Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs(4 hunks)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs(2 hunks)Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs(1 hunks)Ginger/GingerCoreNET/Run/RunsetExecutor.cs(4 hunks)Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (21)
📚 Learning: 2025-07-09T13:44:51.210Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs:658-667
Timestamp: 2025-07-09T13:44:51.210Z
Learning: In UCDataMapping.xaml.cs SetDatabaseValues() method, the line xDBValueExpression.Visibility = Visibility.Visible is intentionally set unconditionally and works correctly with the visibility logic in SetValueControlsView(). This is the expected behavior per user confirmation.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xamlGinger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-07-09T13:45:55.367Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs:230-234
Timestamp: 2025-07-09T13:45:55.367Z
Learning: In UCDataMapping.xaml.cs ValueExpression binding code, using WorkSpace.Instance.SolutionRepository.GetAllRepositoryItems<ProjEnvironment>().FirstOrDefault() without null checks for the mContext creation is intentional and working as expected per user confirmation.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xamlGinger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1123-1126
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has removed an unnecessary empty line at 1123 in `WindowExplorerPage.xaml.cs` for better code compactness.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml
📚 Learning: 2025-07-09T13:46:53.701Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/Ginger/Actions/ActionEditPages/ValidationDBPage.xaml.cs:142-143
Timestamp: 2025-07-09T13:46:53.701Z
Learning: In ValidationDBPage.xaml.cs, the new constructor overload that accepts SelectedContentArgs and ActDBValidation intentionally removes the SelectionChanged event handlers for AppNameComboBox and DBNameComboBox without restoring them. This is expected behavior since this constructor sets up the combo boxes with fixed values and makes them non-editable, so the normal selection changed behavior is not needed.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xamlGinger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-04-01T06:48:16.412Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 4150
File: Ginger/GingerCoreNET/Application Models/Delta/PomDelta/MultiPomRunSetMapping.cs:28-28
Timestamp: 2025-04-01T06:48:16.412Z
Learning: The property in MultiPomRunSetMapping class was renamed from 'runSetConfig' to 'RunSetConfig' to follow C# PascalCase naming conventions for public properties.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.csGinger/GingerCoreNET/Run/RunsetExecutor.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 0
File: :0-0
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The comment regarding the use of `referenceActivity.Guid` instead of `itemCopy.Guid` in the `MarkActivityAsLink` method is considered outdated by the user IamRanjeetSingh.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
📚 Learning: 2025-07-22T12:18:44.463Z
Learnt from: Maheshkale447
Repo: Ginger-Automation/Ginger PR: 4259
File: Ginger/GingerCoreNET/RunLib/CLILib/CLIHelper.cs:1065-1094
Timestamp: 2025-07-22T12:18:44.463Z
Learning: AutoRunWizard in Ginger/Ginger/RunSetLib/CreateAutoRunWizardLib/AutoRunWizard.cs is a configuration wizard that creates CLI configurations and shortcuts, but does not use temp folder functionality from CLIHelper, so it does not need temp folder disposal logic.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
📚 Learning: 2025-01-21T11:43:12.379Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 4067
File: Ginger/GingerCoreCommon/Repository/SolutionRepository.cs:55-55
Timestamp: 2025-01-21T11:43:12.379Z
Learning: In Ginger's SolutionRepository, the path @"ExecutionResults\GingerExecutionResults.db" needs to be explicitly excluded in mSolutionPathsToAvoid list, even though its parent directory @"ExecutionResults\" is also excluded. This specific file exclusion serves a distinct purpose and should not be considered redundant.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
📚 Learning: 2025-09-13T07:36:47.950Z
Learnt from: AmanPrasad43
Repo: Ginger-Automation/Ginger PR: 4301
File: Ginger/GingerCoreCommon/WorkSpaceLib/Solution.cs:499-503
Timestamp: 2025-09-13T07:36:47.950Z
Learning: In the Ginger project, CLI mode can be detected using WorkSpace.Instance.GingerCLIMode enum instead of checking if UserProfile is null. The user AmanPrasad43 added a UserProfile null check in Solution.cs PostDeserialization method to avoid creating GingerPlay configuration during CLI execution, which is a valid concern for CLI scenarios.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
📚 Learning: 2024-10-15T07:06:51.444Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 3958
File: Ginger/Ginger/App.xaml.cs:246-246
Timestamp: 2024-10-15T07:06:51.444Z
Learning: In `Ginger/Ginger/App.xaml.cs`, the `cliProcessor` field is used in multiple methods (`ParseCommandLineArguments` and `RunNewCLI`), so it should remain as a class-level field.
Applied to files:
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs
📚 Learning: 2024-11-29T07:45:52.303Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4011
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:102-105
Timestamp: 2024-11-29T07:45:52.303Z
Learning: When reviewing the password encryption implementation in `GingerCoreNET/RunLib/CLILib`, consider that the existing approach is acceptable, and changes to enhance security aspects are not required.
Applied to files:
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs
📚 Learning: 2025-03-20T10:08:02.309Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4137
File: Ginger/GingerCoreCommon/SourceControlLib/SourceControlBase.cs:58-59
Timestamp: 2025-03-20T10:08:02.309Z
Learning: Plain string storage for credentials in the SourceControlBase class is acceptable for now as confirmed by the developer. Security improvements for credential storage may be planned for a future iteration.
Applied to files:
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs
📚 Learning: 2025-07-09T14:46:34.133Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4245
File: Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs:1533-1535
Timestamp: 2025-07-09T14:46:34.133Z
Learning: In the ValueExpression.cs ReplaceEnvDBWithValue() method, error cases that return without replacing the original expression in mValueCalculated are intentional behavior and working as expected, rather than being inconsistent error handling that should be fixed.
Applied to files:
Ginger/GingerCoreNET/Run/RunsetExecutor.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1139-1142
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has updated the code by removing or addressing the commented code as suggested in the review.
Applied to files:
Ginger/GingerCoreNET/Run/RunsetExecutor.cs
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: IamRanjeetSingh
Repo: Ginger-Automation/Ginger PR: 3488
File: Ginger/GingerCoreNET/DataSource/LiteDB.cs:53-60
Timestamp: 2024-07-26T22:04:12.930Z
Learning: User IamRanjeetSingh has reviewed and accepted the side effects of triggering `TryUpgradeDataFile` upon setting `FileFullPath` in `GingerLiteDB` class.
Applied to files:
Ginger/GingerCoreNET/Run/RunsetExecutor.cs
📚 Learning: 2025-04-30T13:21:34.994Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 4193
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/AIGeneratedPreviewWizardPage.xaml.cs:131-138
Timestamp: 2025-04-30T13:21:34.994Z
Learning: In AIGeneratedPreviewWizardPage.xaml.cs, the current code checks if the response ends with triple backticks and removes them manually. This method is fragile and should be replaced with a more robust regex-based parser. The user plans to address this in a future update.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-07-10T07:11:28.974Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 4249
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:4684-4695
Timestamp: 2025-07-10T07:11:28.974Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, attribute values used in CSS selectors (e.g., ByTitle, ByAriaLabel, ByDataTestId, ByPlaceholder) should be escaped using a helper method (such as EscapeCssAttributeValue) to prevent selector breakage due to special characters.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-03-20T11:10:30.816Z
Learnt from: GokulBothe99
Repo: Ginger-Automation/Ginger PR: 4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
Repo: Ginger-Automation/Ginger PR: 4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs
🧬 Code graph analysis (6)
Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (2)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)Ginger/GingerCoreCommon/Repository/BusinessFlowLib/BusinessFlow.cs (3)
VariableBase(665-669)VariableBase(671-699)VariableBase(701-720)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (6)
Ginger/GingerCoreCommon/Repository/RepositoryItemBase.cs (2)
OnPropertyChanged(208-216)PostDeserialization(1482-1484)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (4)
GetInitialValue(124-127)ResetValue(62-65)VariablePasswordString(25-132)VariablePasswordString(27-29)Ginger/GingerCoreCommon/VariablesLib/VariableDateTime.cs (3)
GetInitialValue(258-261)ResetValue(208-211)VariableDateTime(26-262)Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs (3)
GetInitialValue(344-347)ResetValue(319-322)VariableNumber(26-348)Ginger/GingerCoreCommon/VariablesLib/VariableString.cs (4)
GetInitialValue(108-111)ResetValue(68-71)VariableString(25-112)VariableString(27-29)Ginger/GingerCoreCommon/VariablesLib/VariableList.cs (1)
ResetValue(75-85)
Ginger/GingerCoreNET/RunLib/DynamicExecutionLib/DynamicExecutionManager.cs (5)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (1)
VariableBase(39-870)Ginger/GingerCoreCommon/Repository/BusinessFlowLib/Activity.cs (1)
VariableBase(678-682)Ginger/GingerCoreCommon/Repository/BusinessFlowLib/BusinessFlow.cs (3)
VariableBase(665-669)VariableBase(671-699)VariableBase(701-720)Ginger/GingerCoreCommon/WorkSpaceLib/Solution.cs (2)
Solution(43-657)Solution(54-57)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (1)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (6)
List(318-318)List(775-775)VariableBase(39-870)SetValue(762-773)ResetValue(309-309)GetInitialValue(834-837)
Ginger/GingerCoreNET/Run/RunsetExecutor.cs (2)
Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (4)
VariableBase(39-870)GetVariableSourceValueForDeserialization(224-276)GetInitialValue(834-837)SetInitialValue(323-323)Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (4)
GetInitialValue(124-127)SetInitialValue(112-122)VariablePasswordString(25-132)VariablePasswordString(27-29)
Ginger/Ginger/UserControlsLib/UCDataMapping.xaml.cs (5)
Ginger/GingerCore/GeneralLib/General.cs (3)
EnableComboItem(818-831)DisableComboItem(782-795)CheckComboItemExist(384-405)Ginger/GingerCoreCommon/VariablesLib/VariableBase.cs (5)
ToString(69-72)VariableBase(39-870)ObservableList(816-832)GetFormula(307-307)SetValue(762-773)Ginger/GingerCoreCommon/VariablesLib/VariableDateTime.cs (2)
VariableDateTime(26-262)CheckDateTimeWithInRange(222-237)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreCommon/VariablesLib/VariableNumber.cs (1)
VariableNumber(26-348)
🔇 Additional comments (3)
Ginger/GingerCoreCommon/VariablesLib/VariablePasswordString.cs (2)
79-89: Password SetValue now safe and ops surfaced — LGTM.
- SetValue encrypts plaintext and keeps Password/Value in sync.
- GetSupportedOperations returns concrete ops; avoids NotImplementedException.
Please add unit tests:
- plaintext -> encrypted SetValue
- already-encrypted passthrough
- SetValue(null/empty) behavior
Also applies to: 91-110
124-128: Confirm “initial” semantics for passwords.GetInitialValue returns Password (current encrypted value). If “initial” is expected to mean the pre-run/original value, confirm this matches VariableBase consumers; otherwise consider storing a distinct initial snapshot.
Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
269-283: Backward-compat mapping conversion looks correctConverting (DiffrentFromOrigin=true, MappedOutputType=None) to Value with a derived source is a safe, explicit path. Good use of the shared extractor.
Also applies to: 331-344
Comment on lines
+1059
to
+1061
| { | ||
| throw new InvalidEnumArgumentException("Not a valid value"); | ||
| } |
Contributor
There was a problem hiding this comment.
Overly strict: throwing on unknown Sealights entity level can break runs
Default should not throw; log and no-op (or choose a safe default).
- default:
- {
- throw new InvalidEnumArgumentException("Not a valid value");
- }
+ default:
+ {
+ Reporter.ToLog(eLogLevel.WARN, $"Unknown Sealights entity level '{WorkSpace.Instance.Solution.SealightsConfiguration.SealightsReportedEntityLevel}'. No tests disabled.");
+ break;
+ }🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Run/RunsetExecutor.cs around lines 1059-1061, the code
currently throws InvalidEnumArgumentException for an unknown Sealights entity
level; instead, change this to log a warning (including the unexpected value)
and either no-op or map to a safe default (e.g., treat as "None"/unknown) so the
run continues. Use the existing logger instance, include contextual info (entity
id/name and the invalid value), and avoid throwing so execution is not aborted;
ensure subsequent logic checks for the safe default/unknown case and behaves
safely.
Maheshkale447
deleted the
runset_variable_customization_simplification
branch
November 12, 2025 12:05
15 tasks
15 tasks
15 tasks
15 tasks
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
3 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.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
UI/UX Changes