Improve password variable handling and reset logic#4470
Merged
ravirk91 merged 3 commits intoMar 20, 2026
Conversation
VariablePasswordString now tracks initial value for proper reset. Password setter only sets initial value once. SetInitialValue stores encrypted value as initial and current. ActSetVariableValue supports updating password variables. Unit test updated to verify encryption/decryption. Password variables now selectable in UI.
Contributor
WalkthroughUI filtering for password variables was relaxed, VariablePasswordString gained persistent initial-value handling and reset behavior, ActSetVariableValue now sets password variables via Var.SetValue, and unit tests were added/updated to validate encrypted storage and correct decryption. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (ActSetVariableValuePage)
participant Action as ActSetVariableValue
participant Var as VariablePasswordString
participant Enc as EncryptionHandler
rect rgba(200,230,255,0.5)
UI->>Action: User triggers SetValue for selected VariablePasswordString
Action->>Var: call SetValue(calculatedValue)
Var->>Enc: encrypt(calculatedValue) (internally)
Enc-->>Var: encryptedValue
Var-->>Action: confirm saved (Value updated, mInitialPassword set if needed)
Action-->>UI: success / refresh UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs`:
- Around line 203-206: Update the VariablePasswordString branch to check the
boolean returned by Var.SetValue(calculatedValue) like the VariableSelectionList
branch does: call Var.SetValue and if it returns false, set the action error
(mError) with a clear message including the variable name (Var.Name) and return
false; otherwise continue as before. This keeps handling consistent and
defensive in case VariablePasswordString.SetValue changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c92de918-7dec-454b-a131-7252d415a773
📒 Files selected for processing (4)
Ginger/Ginger/Actions/ActionEditPages/ActSetVariableValuePage.xaml.csGinger/GingerCoreCommon/VariablesLib/VariablePasswordString.csGinger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.csGinger/GingerCoreNETUnitTest/NonUITests/GingerVariableTests.cs
Comment on lines
+203
to
+206
| else if (Var.GetType() == typeof(VariablePasswordString)) | ||
| { | ||
| Var.SetValue(calculatedValue); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider checking the return value of SetValue for consistency.
Other branches like VariableSelectionList (line 130-135) check the result and set an error message on failure. While VariablePasswordString.SetValue currently always returns true (since SupportSetValue is hardcoded to true), checking the return value maintains consistency and provides defensive handling if the implementation changes.
Suggested improvement
else if (Var.GetType() == typeof(VariablePasswordString))
{
- Var.SetValue(calculatedValue);
+ if (!Var.SetValue(calculatedValue))
+ {
+ Error = "Failed to set password variable '" + Var.Name + "'.";
+ return;
+ }
}📝 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
| else if (Var.GetType() == typeof(VariablePasswordString)) | |
| { | |
| Var.SetValue(calculatedValue); | |
| } | |
| else if (Var.GetType() == typeof(VariablePasswordString)) | |
| { | |
| if (!Var.SetValue(calculatedValue)) | |
| { | |
| Error = "Failed to set password variable '" + Var.Name + "'."; | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNET/ActionsLib/General/ActSetVariableValue.cs` around lines
203 - 206, Update the VariablePasswordString branch to check the boolean
returned by Var.SetValue(calculatedValue) like the VariableSelectionList branch
does: call Var.SetValue and if it returns false, set the action error (mError)
with a clear message including the variable name (Var.Name) and return false;
otherwise continue as before. This keeps handling consistent and defensive in
case VariablePasswordString.SetValue changes.
added 2 commits
March 20, 2026 12:05
…ble-set-password-type-variable_EEBT
Updated activity-level test for Password String Set Value support and added new tests for Business Flow and Global levels. Tests verify encrypted storage and correct value resolution at all supported scopes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Ginger/GingerCoreNETUnitTest/NonUITests/GingerVariableTests.cs`:
- Around line 234-238: The test currently only checks that v1.Value differs from
initialValue and that DecryptwithKey(v1.Value) equals newValue; add an assertion
that v1.Value is not equal to newValue to ensure the stored value is not
plaintext (i.e., Assert.AreNotEqual(newValue, v1.Value)), keeping the existing
Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)) to verify
proper decryption; reference v1.Value, initialValue, newValue and
EncryptionHandler.DecryptwithKey when making this change.
- Around line 290-314: Save the current BusinessFlow.SolutionVariables before
setting it (capture its original value), and in the existing finally block
restore it after removing v1 so the test does not leave global state changed;
locate the assignment to BusinessFlow.SolutionVariables and add a local variable
to hold the original, then assign BusinessFlow.SolutionVariables back to that
original in the finally block alongside solution.Variables.Remove(v1).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 998fb109-df12-445d-ab94-c48e47e94bc8
📒 Files selected for processing (1)
Ginger/GingerCoreNETUnitTest/NonUITests/GingerVariableTests.cs
Comment on lines
+234
to
+238
| // Set Variable Action did not support Password String variables earlier (variable not listed / value not set at runtime). | ||
| // Support was added: Set Value encrypts and stores the new password. The test was updated accordingly— | ||
| // we assert the stored value is no longer the plain initial value, then decrypt and compare to the configured new value. | ||
| Assert.AreNotEqual(initialValue, v1.Value); | ||
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen encryption assertion by also checking against newValue.
The current assertion Assert.AreNotEqual(initialValue, v1.Value) only confirms the value changed from the initial value. To truly verify the value is stored encrypted (not plaintext), also assert that the stored value differs from newValue:
Assert.AreNotEqual(initialValue, v1.Value);
+Assert.AreNotEqual(newValue, v1.Value);
Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value));This ensures the test fails if encryption is accidentally bypassed and plaintext is stored.
📝 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
| // Set Variable Action did not support Password String variables earlier (variable not listed / value not set at runtime). | |
| // Support was added: Set Value encrypts and stores the new password. The test was updated accordingly— | |
| // we assert the stored value is no longer the plain initial value, then decrypt and compare to the configured new value. | |
| Assert.AreNotEqual(initialValue, v1.Value); | |
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); | |
| // Set Variable Action did not support Password String variables earlier (variable not listed / value not set at runtime). | |
| // Support was added: Set Value encrypts and stores the new password. The test was updated accordingly— | |
| // we assert the stored value is no longer the plain initial value, then decrypt and compare to the configured new value. | |
| Assert.AreNotEqual(initialValue, v1.Value); | |
| Assert.AreNotEqual(newValue, v1.Value); | |
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNETUnitTest/NonUITests/GingerVariableTests.cs` around lines
234 - 238, The test currently only checks that v1.Value differs from
initialValue and that DecryptwithKey(v1.Value) equals newValue; add an assertion
that v1.Value is not equal to newValue to ensure the stored value is not
plaintext (i.e., Assert.AreNotEqual(newValue, v1.Value)), keeping the existing
Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)) to verify
proper decryption; reference v1.Value, initialValue, newValue and
EncryptionHandler.DecryptwithKey when making this change.
Comment on lines
+290
to
+314
| BusinessFlow.SolutionVariables = WorkSpace.Instance.Solution.Variables; | ||
|
|
||
| VariablePasswordString v1 = new VariablePasswordString() { Name = variableName, Password = initialValue }; | ||
| solution.Variables.Add(v1); | ||
|
|
||
| Activity activity1 = new Activity() { Active = true }; | ||
| mBF.Activities.Add(activity1); | ||
|
|
||
| ActSetVariableValue actSetVariableValue = new ActSetVariableValue() { VariableName = variableName, SetVariableValueOption = VariableBase.eSetValueOptions.SetValue, Value = newValue, Active = true }; | ||
| activity1.Acts.Add(actSetVariableValue); | ||
|
|
||
| try | ||
| { | ||
| mGR.Executor.RunRunner(); | ||
|
|
||
| Assert.AreEqual(eRunStatus.Passed, mBF.RunStatus); | ||
| Assert.AreEqual(eRunStatus.Passed, activity1.Status); | ||
| // Password variables store the runtime value encrypted (VariablePasswordString.SetValue). | ||
| Assert.AreNotEqual(initialValue, v1.Value); | ||
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); | ||
| } | ||
| finally | ||
| { | ||
| solution.Variables.Remove(v1); | ||
| } |
Contributor
There was a problem hiding this comment.
Consider restoring BusinessFlow.SolutionVariables in the finally block.
Line 290 sets the static property BusinessFlow.SolutionVariables, but this is not restored after the test. If other tests depend on this property having a different (or null) value, test isolation may be compromised.
🛡️ Proposed fix to preserve test isolation
+ var originalSolutionVariables = BusinessFlow.SolutionVariables;
BusinessFlow.SolutionVariables = WorkSpace.Instance.Solution.Variables;
VariablePasswordString v1 = new VariablePasswordString() { Name = variableName, Password = initialValue };
solution.Variables.Add(v1);
...
finally
{
solution.Variables.Remove(v1);
+ BusinessFlow.SolutionVariables = originalSolutionVariables;
}📝 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
| BusinessFlow.SolutionVariables = WorkSpace.Instance.Solution.Variables; | |
| VariablePasswordString v1 = new VariablePasswordString() { Name = variableName, Password = initialValue }; | |
| solution.Variables.Add(v1); | |
| Activity activity1 = new Activity() { Active = true }; | |
| mBF.Activities.Add(activity1); | |
| ActSetVariableValue actSetVariableValue = new ActSetVariableValue() { VariableName = variableName, SetVariableValueOption = VariableBase.eSetValueOptions.SetValue, Value = newValue, Active = true }; | |
| activity1.Acts.Add(actSetVariableValue); | |
| try | |
| { | |
| mGR.Executor.RunRunner(); | |
| Assert.AreEqual(eRunStatus.Passed, mBF.RunStatus); | |
| Assert.AreEqual(eRunStatus.Passed, activity1.Status); | |
| // Password variables store the runtime value encrypted (VariablePasswordString.SetValue). | |
| Assert.AreNotEqual(initialValue, v1.Value); | |
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); | |
| } | |
| finally | |
| { | |
| solution.Variables.Remove(v1); | |
| } | |
| var originalSolutionVariables = BusinessFlow.SolutionVariables; | |
| BusinessFlow.SolutionVariables = WorkSpace.Instance.Solution.Variables; | |
| VariablePasswordString v1 = new VariablePasswordString() { Name = variableName, Password = initialValue }; | |
| solution.Variables.Add(v1); | |
| Activity activity1 = new Activity() { Active = true }; | |
| mBF.Activities.Add(activity1); | |
| ActSetVariableValue actSetVariableValue = new ActSetVariableValue() { VariableName = variableName, SetVariableValueOption = VariableBase.eSetValueOptions.SetValue, Value = newValue, Active = true }; | |
| activity1.Acts.Add(actSetVariableValue); | |
| try | |
| { | |
| mGR.Executor.RunRunner(); | |
| Assert.AreEqual(eRunStatus.Passed, mBF.RunStatus); | |
| Assert.AreEqual(eRunStatus.Passed, activity1.Status); | |
| // Password variables store the runtime value encrypted (VariablePasswordString.SetValue). | |
| Assert.AreNotEqual(initialValue, v1.Value); | |
| Assert.AreEqual(newValue, EncryptionHandler.DecryptwithKey(v1.Value)); | |
| } | |
| finally | |
| { | |
| solution.Variables.Remove(v1); | |
| BusinessFlow.SolutionVariables = originalSolutionVariables; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Ginger/GingerCoreNETUnitTest/NonUITests/GingerVariableTests.cs` around lines
290 - 314, Save the current BusinessFlow.SolutionVariables before setting it
(capture its original value), and in the existing finally block restore it after
removing v1 so the test does not leave global state changed; locate the
assignment to BusinessFlow.SolutionVariables and add a local variable to hold
the original, then assign BusinessFlow.SolutionVariables back to that original
in the finally block alongside solution.Variables.Remove(v1).
ravirk91
deleted the
Feature/48481_not-able-set-password-type-variable_EEBT
branch
March 20, 2026 09:39
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
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.
VariablePasswordString now tracks initial value for proper reset. Password setter only sets initial value once. SetInitialValue stores encrypted value as initial and current. ActSetVariableValue supports updating password variables. Unit test updated to verify encryption/decryption. Password variables now selectable in UI.
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit
Bug Fixes
Tests