Conversation
Update key parameter naming, and add observations to voucher fields
…n fields Added new parameters for usage limit, start, and expiration to voucher creation methods in both the API controller and model. Updated relevant methods to handle these new fields
|
Warning Rate limit exceeded@joamag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe changes introduce enhanced voucher management features. The voucher retrieval endpoint now uses a string key instead of an integer ID. Voucher creation methods support additional parameters such as usage limits, start and expiration dates, and metadata. New endpoints allow for using and disusing vouchers, with usage logging implemented via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIController as VoucherAPIController
participant VoucherModel as Voucher
participant VoucherUseModel as VoucherUse
Client->>APIController: POST /api/vouchers/<key>/use (amount, currency, justification, save_use)
APIController->>VoucherModel: use_s(amount, currency, justification, save_use)
alt save_use is True
VoucherModel->>VoucherUseModel: create usage record (amount, justification, voucher, account)
end
VoucherModel-->>APIController: updated voucher, usage record (if any)
APIController-->>Client: JSON response with voucher and usage info
Client->>APIController: POST /api/vouchers/<key>/disuse
APIController->>VoucherModel: disuse_s()
VoucherModel-->>APIController: updated voucher
APIController-->>Client: JSON response with updated voucher
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the voucher system by adding richer configuration and usage tracking.
- Extended Voucher model with
usage_limit,start,expiration, andmetaattributes and updated the factory operations to accept them. - Introduced the
VoucherUsemodel to record usage details and augmenteduse_s/disuse_sto persist usage. - Updated API routes to look up vouchers by key and added
/useand/disuseendpoints.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/budy/models/voucher_use.py | Added VoucherUse model for recording voucher usage |
| src/budy/models/voucher.py | Extended Voucher with new fields/operations and updated use_s/disuse_s |
| src/budy/models/init.py | Registered VoucherUse in the models package |
| src/budy/controllers/api/voucher.py | Changed voucher lookup to key and added /use//disuse API endpoints |
Comments suppressed due to low confidence (1)
src/budy/controllers/api/voucher.py:54
- Change the route parameter from
<int:key>to<str:key>(or omit the type) so voucher keys (which are strings) can be matched correctly.
@appier.route("/api/vouchers/<int:key>", "GET", json=True)
| observations="""Justification or reason for the usage of the voucher,m | ||
| may contain external ID references""", |
There was a problem hiding this comment.
Fix the typo in the observations comment: change voucher,m to voucher, may (or remove the comma) to correct the spacing.
| observations="""Justification or reason for the usage of the voucher,m | |
| may contain external ID references""", | |
| observations="""Justification or reason for the usage of the voucher, may | |
| contain external ID references""", |
| self.used = False | ||
|
|
||
| def use_s(self, amount, currency=None): | ||
| def use_s(self, amount, currency=None, justification=None, save_use=True): |
There was a problem hiding this comment.
The use_s method accepts a currency parameter but later passes it to VoucherUse, which does not define a currency field, resulting in an unexpected keyword argument error.
| def use_s(self, amount, currency=None, justification=None, save_use=True): | ||
| amount_l = self.to_local(amount, currency) | ||
| appier.verify(self.is_valid(amount=amount, currency=currency)) | ||
| if self.is_value and not self.unlimited: | ||
| self.used_amount += commons.Decimal(amount_l) | ||
| self.usage_count += 1 | ||
| self.save() | ||
| if save_use: | ||
| voucher_use_ = voucher_use.VoucherUse( | ||
| voucher=self, | ||
| amount=amount, | ||
| currency=currency, | ||
| justification=justification, | ||
| ) | ||
| voucher_use_.save() | ||
| return voucher_use_ |
There was a problem hiding this comment.
Remove or replace the currency argument when creating VoucherUse, and set usage_type (e.g., VoucherUse.USAGE_TYPE_S['value'] or ['percentage']) to record the usage type correctly.
| def use_s(self, amount, currency=None, justification=None, save_use=True): | |
| amount_l = self.to_local(amount, currency) | |
| appier.verify(self.is_valid(amount=amount, currency=currency)) | |
| if self.is_value and not self.unlimited: | |
| self.used_amount += commons.Decimal(amount_l) | |
| self.usage_count += 1 | |
| self.save() | |
| if save_use: | |
| voucher_use_ = voucher_use.VoucherUse( | |
| voucher=self, | |
| amount=amount, | |
| currency=currency, | |
| justification=justification, | |
| ) | |
| voucher_use_.save() | |
| return voucher_use_ | |
| def use_s(self, amount, justification=None, save_use=True): | |
| amount_l = self.to_local(amount, self.currency) | |
| appier.verify(self.is_valid(amount=amount, currency=self.currency)) | |
| if self.is_value and not self.unlimited: | |
| self.used_amount += commons.Decimal(amount_l) | |
| self.usage_count += 1 | |
| self.save() | |
| if save_use: | |
| usage_type = ( | |
| voucher_use.VoucherUse.USAGE_TYPE_S["value"] | |
| if self.is_value | |
| else voucher_use.VoucherUse.USAGE_TYPE_S["percentage"] | |
| ) | |
| voucher_use_ = voucher_use.VoucherUse( | |
| voucher=self, | |
| amount=amount, | |
| usage_type=usage_type, | |
| justification=justification, | |
| ) | |
| voucher_use_.save() | |
| return voucher_use_ |
| currency = self.field("currency", currency, cast=str) | ||
| unlimited = self.field("unlimited", unlimited, cast=bool) | ||
| voucher = budy.Voucher.create_value_s(key, amount, currency, unlimited) | ||
| usage_limit = object.get("usage_limit", 0) |
There was a problem hiding this comment.
Consider using self.field(...) or similar validation helpers to cast and validate usage_limit, start, expiration, and meta inputs consistently, as done with the original create_value parameters.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/budy/models/voucher.py (1)
93-141: Add missingmetafield
metais assigned in every constructor path but noappier.fieldexists, causing it to be stored as a volatile attribute only.@@ unlimited = appier.field( type=bool, initial=False, index=True, safe=True, observations="""Flag that indicated if the value based voucher should not have its used amount deducted and instead be considered an unlimited voucher""", ) + + meta = appier.field( + type=dict, + safe=True, + observations="""Arbitrary structured metadata associated with the voucher""", + )
🧹 Nitpick comments (4)
src/budy/models/__init__.py (1)
28-56: Missing symmetric module import forvoucher_useAll other models are imported twice – once via
from . import <module>(for side-effects /__all__discovery) and again viafrom .<module> import <Class>.
voucher_usewas only added to the second section. Add the first-stage import to keep the package initialisation pattern consistent and to ensure side-effects (signals, monkey-patches, etc.) execute.@@ from . import voucher +from . import voucher_usesrc/budy/models/voucher_use.py (2)
57-62: Typo in observation string
voucher,mcontains a straym.- observations="""Justification or reason for the usage of the voucher,m + observations="""Justification or reason for the usage of the voucher,
38-41: Minor: prefer literal dict overdict()USAGE_TYPE_S = {"percentage": "percentage", "value": "value"}Pure style; feel free to ignore if you follow a different convention.
src/budy/models/voucher.py (1)
331-347: Inconsistent return behaviour flagged by pylint
use_sreturns aVoucherUseinstance in one branch andNonein the other. Consider always returning the created (orNone) object explicitly, or returningselfwhensave_use=Falseto make the API intention clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/budy/controllers/api/voucher.py(3 hunks)src/budy/models/__init__.py(1 hunks)src/budy/models/voucher.py(6 hunks)src/budy/models/voucher_use.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/budy/models/__init__.py (1)
src/budy/models/voucher_use.py (1)
VoucherUse(36-96)
src/budy/models/voucher.py (1)
src/budy/models/voucher_use.py (1)
VoucherUse(36-96)
🪛 Ruff (0.11.9)
src/budy/models/__init__.py
87-87: .voucher_use.VoucherUse imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🪛 Pylint (3.3.7)
src/budy/models/voucher_use.py
[refactor] 38-41: Consider using '{"percentage": 'percentage', "value": 'value'}' instead of a call to 'dict'.
(R1735)
src/budy/models/voucher.py
[refactor] 41-41: Too many public methods (28/20)
(R0904)
[refactor] 208-208: Too many arguments (9/5)
(R0913)
[refactor] 208-208: Too many positional arguments (9/5)
(R0917)
[refactor] 239-239: Too many arguments (10/5)
(R0913)
[refactor] 239-239: Too many positional arguments (10/5)
(R0917)
[refactor] 278-278: Too many arguments (7/5)
(R0913)
[refactor] 278-278: Too many positional arguments (7/5)
(R0917)
[refactor] 303-303: Too many arguments (8/5)
(R0913)
[refactor] 303-303: Too many positional arguments (8/5)
(R0917)
[refactor] 331-331: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🔇 Additional comments (3)
src/budy/models/__init__.py (1)
87-87: Static-analysis false positive can be ignored
VoucherUseis deliberately imported for re-export (access viabudy.VoucherUse). No further action is required; suppress the Ruff F401 rule in your config if it is too noisy.src/budy/controllers/api/voucher.py (1)
67-75:metaaccepted by the API but not persisted
metais forwarded toVoucher.create_value_s, yetVoucherhas nometafield.
Either add the field to the model or drop the parameter here to avoid user confusion.src/budy/models/voucher.py (1)
331-346:VoucherUsereceives an undefinedcurrencyargumentWith the added
currencyfield inVoucherUse, this becomes valid. If you decide not to store currency, remove the argument here to avoid a runtimeTypeErrorwhen strict constructors are enabled.
Implemented a new test case to validate the usage of vouchers, including checks for validity, remaining amounts, and database retrieval of usage records.
…to VoucherUse model
This pull request introduces significant enhancements to the voucher system in the
budyapplication. The changes include extending voucher functionality with new attributes, operations, and validations, as well as introducing a newVoucherUsemodel to track voucher usage. These updates aim to provide more flexibility and control over voucher creation, management, and usage.Enhancements to voucher functionality:
Expanded voucher attributes and operations:
usage_limit,start,expiration, andmetato theVouchermodel to support more complex voucher configurations. These attributes allow setting limits, validity periods, and metadata for vouchers. (src/budy/models/voucher.py, [1] [2] [3] [4] [5]create_value_s,create_value_multiple_s,create_percentage_s, andcreate_percentage_multiple_sto incorporate the new attributes. (src/budy/models/voucher.py, [1] [2] [3]New API endpoints for voucher usage:
useanddisuseendpoints to manage voucher usage. Theuseendpoint supports saving usage data with optional justification, while thedisuseendpoint reverses voucher usage. (src/budy/controllers/api/voucher.py, src/budy/controllers/api/voucher.pyL82-R115)Introduction of
VoucherUsemodel:VoucherUsemodel to record detailed information about voucher usage, including amount, justification, and references to the associated voucher and account. (src/budy/models/voucher_use.py, src/budy/models/voucher_use.pyR1-R96)use_smethod inVoucherto optionally save usage details in theVoucherUsemodel. (src/budy/models/voucher.py, src/budy/models/voucher.pyL248-R346)Codebase organization:
VoucherUsemodel to thesrc/budy/models/__init__.pyfile for proper module integration. (src/budy/models/__init__.py, src/budy/models/init.pyR87)Summary by CodeRabbit
New Features
Improvements