Skip to content

fix: reconfigure script prefill#672

Merged
firstof9 merged 2 commits into
FutureTense:mainfrom
ygelfand:fix/reconfigure-notify-script-prefill
Jul 7, 2026
Merged

fix: reconfigure script prefill#672
firstof9 merged 2 commits into
FutureTense:mainfrom
ygelfand:fix/reconfigure-notify-script-prefill

Conversation

@ygelfand

@ygelfand ygelfand commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

When reconfiguring the integration, the previous selected value (if not none) is always lost, because it doesn't match the "script." prefix. This restores it.

Proposed change

Minor change overall, just effects reconfigure, but could silently lose custom notifier if you don't re-select it.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Signed-off-by: ygelfand <yuri@shlitz.com>
@github-actions github-actions Bot added the bugfix Fixes a bug label Jul 6, 2026
@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.58%. Comparing base (cdb4922) to head (95b42d1).
⚠️ Report is 188 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #672      +/-   ##
==========================================
+ Coverage   84.14%   92.58%   +8.44%     
==========================================
  Files          10       41      +31     
  Lines         801     4856    +4055     
  Branches        0       30      +30     
==========================================
+ Hits          674     4496    +3822     
- Misses        127      360     +233     
Flag Coverage Δ
python 92.44% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@secondof9

Copy link
Copy Markdown

Code Review: PR #672 — "fix: reconfigure script prefill"

Summary

This PR adds a defensive normalization step in the reconfigure flow of custom_components/keymaster/config_flow.py to ensure that a previously selected notify script (stored without the script. prefix) is properly prefixed when re-queried against the available scripts list.

Line-by-Line Analysis

custom_components/keymaster/config_flow.py

+ Lines 118-120 (new block):

        notify_script = self._data[CONF_NOTIFY_SCRIPT_NAME]
        if notify_script and notify_script != NONE_TEXT and not notify_script.startswith("script."):
            self._data[CONF_NOTIFY_SCRIPT_NAME] = f"script.{notify_script}"

This adds 4 lines of defensive normalization. The logic reads the stored script name, checks if it is not NONE_TEXT and does not already have the script. prefix, and prepends script. if needed. This ensures the subsequent lookup self._data[CONF_NOTIFY_SCRIPT_NAME] not in notify_scripts matches correctly.

Review Checklist

  • Correctness: ✅ The fix addresses a real bug where stored script names without the script. prefix would fail to match entries in the notify_scripts list during reconfigure. The guard checks != NONE_TEXT and not notify_script.startswith("script.") correctly.
  • Edge cases:NONE_TEXT is handled explicitly. Empty/None values are excluded by the truthiness check. Already-prefixed names are skipped.
  • Security: ✅ No security issues — this is a simple string operation on local data.
  • Quality: ✅ Clean, readable, minimal change. No dead code or side effects.
  • Testing: ℹ️ No test changes. A unit test covering the reconfigure path with a missing script. prefix would be ideal but not blocking.

Verdict

🟢 Looks Good

Well-scoped bugfix. The issue is clear (prefix mismatch on reconfigure), the fix is minimal and correct, and the edge cases are properly guarded. No concerns.

✅ Looks Good

  • The fix is minimal and targeted — exactly what's needed for the reported bug.
  • Defensive checks (!= NONE_TEXT, not startswith("script.")) are present and correct.
  • The string operation f"script.{notify_script}" is safe and readable.
  • Existing error handling (the not in notify_scripts check) still applies after normalization.

Repository owner deleted a comment from secondof9 Jul 6, 2026

@firstof9 firstof9 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unit test covering the reconfigure path with a missing script. prefix would be a great addition.

@firstof9
firstof9 merged commit bf8b6e8 into FutureTense:main Jul 7, 2026
7 checks passed
@firstof9

firstof9 commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants