fix: set values to None so set_missing_values works - #11
Merged
Conversation
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking gap in regression coverage for the newly corrected defaulting behavior. The normalization matches the documented semantics of optional fields and no behavioral or security defect was established, but the interaction with Sales Invoice defaulting is not directly tested. Files Needing Attention: simple_subscription/simple_subscription/doctype/simple_subscription/simple_subscription.py Prompt To Fix All With AI### Issue 1
simple_subscription/simple_subscription/doctype/simple_subscription/simple_subscription.py:118-129
**Defaulting Behavior Lacks Coverage**
These assignments now normalize three optional values to `None` and rely on the later `set_missing_values()` call to populate address and tax defaults. No regression test covers invoice creation with blank versus explicitly selected values, so changes to Frappe's defaulting behavior or this method could silently break the fix.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: simplify syntax and add taxes case ..." | Re-trigger Greptile |
barredterra
approved these changes
Sep 8, 2026
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
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.
The bug:
Surfaced on further testing of the changes introduced in PR #10:
set_missing_values() does not auto-fill empty strings (''), just None. The Problem was already present for the "sales_taxes_and_charges" field.
Impact:
If a value in the simple subscription in for example the "customer_address" field get's set and unset it is '' in the DB, not None. So the Address is then empty after invoice creation.
Further Note on the Taxes Site:
The Bug did only hit when a Pricing Rule existed and matched. Otherwise (for example if only a Default 19% Template was present or if Item Taxes applied it did not bite)