Skip to content

fix: prevent config dialog known-path/boolean data loss#1291

Open
crowecawcaw wants to merge 5 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/config-dialog-dataloss
Open

fix: prevent config dialog known-path/boolean data loss#1291
crowecawcaw wants to merge 5 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/config-dialog-dataloss

Conversation

@crowecawcaw

Copy link
Copy Markdown
Contributor

Fixes:

What was the problem/requirement? (What/Why)

Two bugs in the config dialog (ui/dialogs/deadline_config_dialog.py):

  • Editing a known-asset path to a value that duplicates an existing entry silently deleted the row: the write/refresh happened unconditionally even when the re-insert was skipped as a duplicate → config data loss.
  • A corrupt boolean setting coerced to False instead of falling back to the setting's declared default.

What was the solution? (How)

  • Move the write/refresh inside the if path not in current_paths: guard so the original row is only removed when the re-insert actually happens.
  • On a str2bool ValueError, fall back to str2bool(get_setting_default(...)) rather than False.

What is the impact of this change?

Editing a known-asset path to a duplicate no longer drops the original row, and a corrupt boolean setting uses its intended default.

How was this change tested?

Added red-green tests: editing to a duplicate keeps the original row (plus a control that a normal edit still stages), and a corrupt boolean falls back to the setting default.

  • Have you run the unit tests? Yestest_settings_dialogue.py + test_deadline_config_dialog.py (18 passed).
  • Have you run the integration tests? No.

Was this change documented?

  • No public-contract change.
  • README.md not affected.

Does this PR introduce new dependencies?

  • This PR adds one or more new dependency Python packages.
  • This PR does not add any new dependencies.

Is this a breaking change?

No.

Does this change impact security?

No.

Editing a known-asset path to a value that already exists in the list
would unconditionally stage the rebuilt path list (which excludes the
row being edited) even though the re-insert was skipped as a duplicate.
This silently dropped the original row from the config = data loss.

Guard the write/refresh on the re-insert actually happening.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
A corrupt/garbage stored boolean setting was blindly coerced to False
in the checkbox refresh, ignoring the setting's declared default. For
settings whose default is 'true' (e.g. submitter_update_notification)
this silently flipped the value off.

Fall back to str2bool(get_setting_default(...)) instead.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@github-actions github-actions Bot added the waiting-on-maintainers Waiting on the maintainers to review. label Jul 21, 2026
@crowecawcaw
crowecawcaw marked this pull request as ready for review July 22, 2026 20:21
@crowecawcaw
crowecawcaw requested a review from a team as a code owner July 22, 2026 20:21
Restores the mock_api fixture's get_setting side_effect after the test
instead of leaving the corrupt-value stub in place, fixing the mypy
attribute error on the MagicMock reassignment.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw force-pushed the review-fix/config-dialog-dataloss branch from 5ee0080 to abe1ce0 Compare July 22, 2026 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-maintainers Waiting on the maintainers to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant