-
Notifications
You must be signed in to change notification settings - Fork 170
Add validation to prevent negative values in gift card settings #1492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds server-side numeric validation for gift card configuration fields to disallow negative values, ensuring gift card codes length is at least 1 and expiry years is non-negative. Class diagram for updated gift card settings configurationclassDiagram
class SettingField {
+string key
+string type
+string label
+string description
+any default
+int min_value
}
class GiftcardCodeLengthSetting {
+string key = giftcard_code_length
+string type = integer
+int min_value = 1
}
class GiftcardExpiryYearsSetting {
+string key = giftcard_expiry_years
+string type = integer
+int min_value = 0
}
SettingField <|-- GiftcardCodeLengthSetting
SettingField <|-- GiftcardExpiryYearsSetting
Flow diagram for gift card settings validation on saveflowchart TD
Admin["Admin updates gift card settings"] --> UI["Gift card settings form"]
UI --> Backend["Submit settings to backend"]
Backend --> ValidateCodeLen["Validate giftcard_code_length (min_value = 1)"]
Backend --> ValidateExpiryYears["Validate giftcard_expiry_years (min_value = 0)"]
ValidateCodeLen -->|value >= 1| CodeLenOK["giftcard_code_length valid"]
ValidateCodeLen -->|value < 1| CodeLenError["Reject: code length must be at least 1"]
ValidateExpiryYears -->|value >= 0| ExpiryOK["giftcard_expiry_years valid"]
ValidateExpiryYears -->|value < 0| ExpiryError["Reject: expiry years cannot be negative"]
CodeLenOK --> CheckAll{"All validations passed?"}
ExpiryOK --> CheckAll
CodeLenError --> CheckAll
ExpiryError --> CheckAll
CheckAll -->|yes| Save["Persist settings"]
CheckAll -->|no| ReturnErrors["Return validation errors to UI"]
Save --> UI
ReturnErrors --> UI
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider updating the help text for
giftcard_code_lengthandgiftcard_expiry_yearsto explicitly mention the enforced minimum values so the UI guidance matches the new validation constraints.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider updating the help text for `giftcard_code_length` and `giftcard_expiry_years` to explicitly mention the enforced minimum values so the UI guidance matches the new validation constraints.
## Individual Comments
### Comment 1
<location> `app/eventyay/base/configurations/default_setting.py:2439` </location>
<code_context>
'If you set a number here, gift cards will by default expire at the end of the year after this '
'many years. If you keep it empty, gift cards do not have an explicit expiry date.'
),
+ min_value=0,
),
},
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify semantics of `0` vs empty value for gift card expiry configuration.
With `min_value=0`, a value of 0 is now valid in addition to leaving the field empty. Please make it explicit whether 0 is intended to mean “no expiry”, “expire immediately”, or something else. If only empty/None should mean “no expiry”, consider `min_value=1` and handling empty as the special case; otherwise, ensure the downstream logic clearly distinguishes 0 from empty.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 'If you set a number here, gift cards will by default expire at the end of the year after this ' | ||
| 'many years. If you keep it empty, gift cards do not have an explicit expiry date.' | ||
| ), | ||
| min_value=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Clarify semantics of 0 vs empty value for gift card expiry configuration.
With min_value=0, a value of 0 is now valid in addition to leaving the field empty. Please make it explicit whether 0 is intended to mean “no expiry”, “expire immediately”, or something else. If only empty/None should mean “no expiry”, consider min_value=1 and handling empty as the special case; otherwise, ensure the downstream logic clearly distinguishes 0 from empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds validation to prevent negative values in gift card configuration settings by introducing minimum value constraints on two fields: gift card code length and gift card expiry years.
- Adds
min_value=1validation forgiftcard_lengthto ensure gift card codes have at least one character - Adds
min_value=0validation forgiftcard_expiry_yearsto prevent negative expiry durations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue
Description
Summary by Sourcery
Enforce non-negative configuration values for gift card settings to prevent invalid negative inputs being stored.
Bug Fixes: