Massive refactoring - #85
Open
adamtornhill wants to merge 1 commit into
Open
Conversation
![codescene-staging-delta-analysis[bot]](https://avatars.githubusercontent.com/in/52858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
✅ 6 Quality Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| if __name__ == "__main__": | ||
| main() | ||
|
|
||
|
|
There was a problem hiding this comment.
✅ No longer an issue: Low Cohesion
The number of different responsibilities in this module is no longer above the threshold
Comment on lines
-110
to
-159
| def initialize_purchase_transaction(identity: Identity, transaction: DbTransaction, | ||
| purchase: PurchaseRequest) -> Optional[str]: | ||
| """Initialize purchase transaction""" | ||
| if identity.user is None: | ||
| abort(400, "Must be a vendor or traveller to initialize a purchase") | ||
| if purchase.start_of_validity: | ||
| if purchase.manual_activation: | ||
| abort(400, "You can not specify both manual activation and start of validity") | ||
| if purchase.start_of_validity < expire(-60): | ||
| abort(400, "You can not specify start of validity in the past") | ||
| # TODO check for start of valid values for start_of_validity | ||
| mtb_bearer = get_db_mtb_bearer(identity, purchase.mtb_bearer_id, error_400=True) if purchase.mtb_bearer_id else None | ||
| if purchase.mtb_product_owner: | ||
| try: | ||
| mtb_product_owner = User.get_from_owner_ref(purchase.mtb_product_owner) | ||
| except DoesNotExist: | ||
| abort(400, "Can't access specified mtbProductOwner ") | ||
| if mtb_bearer is not None and mtb_bearer.owner.id != mtb_product_owner.id: | ||
| abort(400, "Specified mtbBearerId must match specified mtbProductOwner") | ||
| else: | ||
| mtb_product_owner = None | ||
| for item in transaction.items: | ||
| if purchase.start_of_validity: | ||
| if item.manual_activation: | ||
| abort(400, "You can not specify manual activation in cart and start of validity in purchase") | ||
| if item.start_of_validity is None: | ||
| item.start_of_validity = purchase.start_of_validity | ||
| elif purchase.manual_activation and item.manual_activation is None: | ||
| item.manual_activation = purchase.manual_activation | ||
| if mtb_bearer is not None: | ||
| if item.mtb_bearer is None: | ||
| item.mtb_bearer = mtb_bearer | ||
| if mtb_product_owner is not None: | ||
| if item.mtb_product_owner is None: | ||
| item.mtb_product_owner = mtb_product_owner | ||
| elif item.mtb_product_owner is None: | ||
| if item.mtb_bearer is not None: | ||
| item.mtb_product_owner = item.mtb_bearer.owner.fetch() | ||
| else: | ||
| item.mtb_product_owner = identity.user | ||
| if item.mtb_bearer is not None and item.mtb_bearer.owner.id != item.mtb_product_owner.id: | ||
| abort(400, "Specified mtbBearerId must match specified mtbProductOwner") | ||
| transaction.owner = identity.user | ||
| if identity.user.type == USER_TYPE_VENDOR: | ||
| transaction.state = TransactionState.FINALIZE_PENDING | ||
| transaction.save() | ||
| webview_url = None | ||
| else: | ||
| webview_url = _initialize_traveller_purchase(identity, purchase, transaction) | ||
| return webview_url |
There was a problem hiding this comment.
✅ No longer an issue: Complex Method
initialize_purchase_transaction is no longer above the threshold for cyclomatic complexity
Comment on lines
-162
to
-235
| def _initialize_traveller_purchase(identity: Identity, purchase: PurchaseRequest, | ||
| transaction: DbTransaction) -> Optional[str]: | ||
| wallet = get_db_wallet(identity, purchase.wallet_id, error_400=True) | ||
| if purchase.payment_method_id is not None: | ||
| payment_method = get_wallet_payment_method(wallet, purchase.payment_method_id) | ||
| else: | ||
| payment_method = None | ||
| if purchase.use_purse is None: | ||
| if wallet.use_purse == USE_PURSE_OPTIONAL: | ||
| use_purse = payment_method is None | ||
| else: | ||
| use_purse = wallet.use_purse == USE_PURSE_ALWAYS | ||
| elif purchase.use_purse: | ||
| if wallet.use_purse == USE_PURSE_NEVER: | ||
| abort(400, "Wallet doesn't allow use of purse") | ||
| use_purse = True | ||
| else: | ||
| if wallet.use_purse == USE_PURSE_ALWAYS: | ||
| abort(400, "Wallet doesn't allow purchases without using purse") | ||
| use_purse = False | ||
| if use_purse: | ||
| new_reservation = NewPurseReservation(amount=float(transaction.total_amount), | ||
| partial=payment_method is not None) | ||
| try: | ||
| reservation = purse_create_reservation(wallet.purse_id, new_reservation) | ||
| transaction.purse_reservation_id = reservation.id | ||
| transaction.purse_amount = from_float_amount(reservation.amount, wallet.currency) | ||
| transaction.expire = reservation.expire | ||
| except ApiException as ae: | ||
| transaction.purse_amount = Decimal(0) | ||
| transaction.purse_reservation_id = None | ||
| # TODO handle other than 409!? | ||
| if ae.status == 409: | ||
| logger.debug("No money in purse") | ||
| else: | ||
| logger.error("Failed to access purse, skipping") | ||
| else: | ||
| transaction.purse_amount = Decimal(0) | ||
| transaction.purse_reservation_id = None | ||
| if transaction.purse_amount < transaction.total_amount: | ||
| if payment_method is not None: | ||
| transaction.payment_method_id = payment_method.id | ||
| transaction.payment_method_amount = transaction.total_amount - transaction.purse_amount | ||
| psd = get_payment_service_driver(get_payment_service(payment_method.payment_service_id)) | ||
| tid = str(transaction.id) | ||
| try: | ||
| payment_transaction = \ | ||
| psd.init_payment(payment_method, transaction.payment_method_amount, tid, | ||
| transaction.description, transaction.currency, | ||
| replace_var("TID", purchase.return_url, tid), | ||
| get_best_lang(identity.user), purchase.mobile) | ||
| except PaymentServiceException as pse: | ||
| logger.error("Failed to initialize payment: %s", pse) | ||
| raise pse | ||
| pt_expire = payment_transaction.get_expire() | ||
| if pt_expire is not None: | ||
| if transaction.expire is None or pt_expire < transaction.expire: | ||
| transaction.expire = pt_expire | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| elif purchase.use_purse: | ||
| abort(400, "Not enough funds available in purse for purchase") | ||
| else: | ||
| abort(400, "No payment method specified") | ||
| webview_url = payment_transaction.get_webview_url() | ||
| else: | ||
| transaction.payment_method_id = None | ||
| transaction.payment_method_transaction_data = None | ||
| transaction.payment_method_amount = None | ||
| webview_url = None | ||
| transaction.wallet = wallet | ||
| transaction.state = TransactionState.USER_INTERACTION_PENDING if webview_url is not None \ | ||
| else TransactionState.FINALIZE_PENDING | ||
| transaction.save() | ||
| return webview_url |
There was a problem hiding this comment.
✅ No longer an issue: Complex Method
_initialize_traveller_purchase is no longer above the threshold for cyclomatic complexity
Comment on lines
-238
to
-316
| def _finalize_traveller_purchase(auth_ctx: AuthorizationContext, transaction: DbTransaction, finalization_data: str) \ | ||
| -> None: | ||
| """Finalize purchase transaction and create receipt""" | ||
| if transaction.payment_method_transaction_data: | ||
| payment_method = get_wallet_payment_method(transaction.wallet, transaction.payment_method_id) | ||
| psd = get_payment_service_driver(get_payment_service(payment_method.payment_service_id)) | ||
| payment_transaction = psd.create_payment_service_transaction(transaction.payment_method_transaction_data) | ||
| try: | ||
| psd.reserve_payment(payment_transaction) | ||
| except PaymentServiceException as pse: | ||
| logger.error("Failed to reserve payment: %s", pse) | ||
| transaction.state = TransactionState.CANCELLED if isinstance(pse, UserInteractionCanceledException) \ | ||
| else TransactionState.DENIED | ||
| _release_purse_reservation(transaction) | ||
| transaction.save() | ||
| raise pse | ||
| else: | ||
| psd = None | ||
| payment_transaction = None | ||
| try: | ||
| _finalize_create_mtb_products(transaction) | ||
| except Exception as exc: | ||
| logger.error("Failed to create product: %s", exc) | ||
| transaction.state = TransactionState.ISSUE_ERROR | ||
| _release_purse_reservation(transaction) | ||
| _release_payment_method_reservation(transaction, psd, payment_transaction) | ||
| _revert_mtb_products(auth_ctx, transaction) | ||
| if payment_transaction: | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| transaction.save() | ||
| raise exc | ||
| if transaction.purse_reservation_id is not None: | ||
| record = NewPurseRecord(transaction_id=str(transaction.id), | ||
| reservation_id=transaction.purse_reservation_id, | ||
| amount=-float(transaction.purse_amount), | ||
| refundable=False) | ||
| try: | ||
| res = purse_create_record(transaction.wallet.purse_id, record) | ||
| except ApiException as ae: | ||
| logger.error("Failed to record purse transaction: %s", ae) | ||
| transaction.state = TransactionState.DENIED | ||
| _revert_mtb_products(auth_ctx, transaction) | ||
| _release_payment_method_reservation(transaction, psd, payment_transaction) | ||
| if payment_transaction: | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| transaction.save() | ||
| raise ae | ||
| transaction.purse_record_ids.append(res.id) | ||
| transaction.purse_reservation_id = None | ||
| if transaction.payment_method_transaction_data: | ||
| try: | ||
| transaction.payment_reference = psd.finalize_payment(payment_transaction, finalization_data) | ||
| except PaymentServiceException as pse: | ||
| logger.error("Failed to reserve payment: %s", pse) | ||
| transaction.state = TransactionState.DENIED | ||
| _revert_mtb_products(auth_ctx, transaction) | ||
| _revert_purse_record(transaction) | ||
| if payment_transaction: | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| transaction.save() | ||
| raise pse | ||
| _mark_purchased(transaction) | ||
| # TODO purge some, transaction.payment_method_transaction_data | ||
| try: | ||
| if payment_transaction: | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| transaction.save() | ||
| except Exception as exc: | ||
| # TODO finer error handling | ||
| transaction.state = TransactionState.ERROR | ||
| _revert_mtb_products(auth_ctx, transaction) | ||
| if transaction.purse_record_ids: | ||
| # TODO revert record? | ||
| pass | ||
| if transaction.payment_method_transaction_data is not None: | ||
| # TODO release payment reservation/payment? | ||
| pass | ||
| transaction.save() | ||
| raise exc |
There was a problem hiding this comment.
✅ No longer an issue: Complex Method
_finalize_traveller_purchase is no longer above the threshold for cyclomatic complexity
Comment on lines
-61
to
-94
| def get_transactions(identity: Identity, after: str = None, before: str = None, mtb_product_id: str = None, | ||
| traveller_id: str = None, vendor_id: str = None, wallet_id: str = None) -> List[Transaction]: | ||
| """Get all transactions matching filter""" | ||
| query = None | ||
| if after: | ||
| query = Q(timestamp__gte=after) | ||
| if before: | ||
| if query is None: | ||
| query = Q(timestamp__lt=before) | ||
| else: | ||
| query = query & Q(timestamp__lt=before) | ||
| if mtb_product_id: | ||
| if query is None: | ||
| query = Q(items__mtb_product_ids=mtb_product_id) | ||
| else: | ||
| query = query & Q(items__mtb_product_ids=mtb_product_id) | ||
| if traveller_id: | ||
| q = Q(owner=db_id(traveller_id)) | Q(recipient=db_id(traveller_id)) | ||
| if query is None: | ||
| query = q | ||
| else: | ||
| query &= q | ||
| if vendor_id: | ||
| q = Q(owner=db_id(vendor_id)) | Q(recipient=db_id(vendor_id)) | ||
| if query is None: | ||
| query = q | ||
| else: | ||
| query &= q | ||
| if wallet_id: | ||
| if query is None: | ||
| query = Q(wallet=db_id(wallet_id)) | ||
| else: | ||
| query = query & Q(wallet=db_id(wallet_id)) | ||
| return [to_transaction_model(_update_cancellable(trans)) for trans in (DbTransaction.objects(q_obj=query))] |
There was a problem hiding this comment.
✅ No longer an issue: Complex Method
get_transactions is no longer above the threshold for cyclomatic complexity
Comment on lines
-421
to
-450
| def cancel_payment_transaction(auth_ctx: AuthorizationContext, transaction: DbTransaction) -> None: | ||
| """Cancel transaction and refund payments and invalidate products""" | ||
| for item in transaction.items: | ||
| if item.mtb_product_ids: | ||
| for mp_id in item.mtb_product_ids: | ||
| try: | ||
| prod = get_db_mtb_product(auth_ctx, mp_id) | ||
| check_mtb_product_useable(auth_ctx, prod) | ||
| except AbortException: | ||
| abort(409, "Transaction contains lent products") | ||
| state = transaction.state | ||
| if state == TransactionState.PURCHASED or state == TransactionState.FINALIZE_PENDING \ | ||
| or state == TransactionState.USER_INTERACTION_PENDING: | ||
| _refund_payment(transaction, "Cancel requested") | ||
| _refund_purse(transaction) | ||
| else: | ||
| abort(501, f"Transaction in state {state} isn't cancellable") | ||
|
|
||
| for item in transaction.items: | ||
| if item.mtb_product_ids: | ||
| for mp_id in item.mtb_product_ids: | ||
| try: | ||
| cancel_mtb_product(auth_ctx, mp_id, transaction) | ||
| except Exception as exc: | ||
| logger.error("Failed to cancel mtb_product {mp_id}", exc_info=exc) | ||
| # TODO remove product from traveller | ||
| transaction.cancellable = False | ||
| transaction.cancellable_expire = None | ||
| transaction.state = TransactionState.CANCELLED | ||
| transaction.save() |
There was a problem hiding this comment.
✅ No longer an issue: Bumpy Road Ahead
cancel_payment_transaction is no longer above the threshold for logical blocks with deeply nested code
Comment on lines
-162
to
-235
| def _initialize_traveller_purchase(identity: Identity, purchase: PurchaseRequest, | ||
| transaction: DbTransaction) -> Optional[str]: | ||
| wallet = get_db_wallet(identity, purchase.wallet_id, error_400=True) | ||
| if purchase.payment_method_id is not None: | ||
| payment_method = get_wallet_payment_method(wallet, purchase.payment_method_id) | ||
| else: | ||
| payment_method = None | ||
| if purchase.use_purse is None: | ||
| if wallet.use_purse == USE_PURSE_OPTIONAL: | ||
| use_purse = payment_method is None | ||
| else: | ||
| use_purse = wallet.use_purse == USE_PURSE_ALWAYS | ||
| elif purchase.use_purse: | ||
| if wallet.use_purse == USE_PURSE_NEVER: | ||
| abort(400, "Wallet doesn't allow use of purse") | ||
| use_purse = True | ||
| else: | ||
| if wallet.use_purse == USE_PURSE_ALWAYS: | ||
| abort(400, "Wallet doesn't allow purchases without using purse") | ||
| use_purse = False | ||
| if use_purse: | ||
| new_reservation = NewPurseReservation(amount=float(transaction.total_amount), | ||
| partial=payment_method is not None) | ||
| try: | ||
| reservation = purse_create_reservation(wallet.purse_id, new_reservation) | ||
| transaction.purse_reservation_id = reservation.id | ||
| transaction.purse_amount = from_float_amount(reservation.amount, wallet.currency) | ||
| transaction.expire = reservation.expire | ||
| except ApiException as ae: | ||
| transaction.purse_amount = Decimal(0) | ||
| transaction.purse_reservation_id = None | ||
| # TODO handle other than 409!? | ||
| if ae.status == 409: | ||
| logger.debug("No money in purse") | ||
| else: | ||
| logger.error("Failed to access purse, skipping") | ||
| else: | ||
| transaction.purse_amount = Decimal(0) | ||
| transaction.purse_reservation_id = None | ||
| if transaction.purse_amount < transaction.total_amount: | ||
| if payment_method is not None: | ||
| transaction.payment_method_id = payment_method.id | ||
| transaction.payment_method_amount = transaction.total_amount - transaction.purse_amount | ||
| psd = get_payment_service_driver(get_payment_service(payment_method.payment_service_id)) | ||
| tid = str(transaction.id) | ||
| try: | ||
| payment_transaction = \ | ||
| psd.init_payment(payment_method, transaction.payment_method_amount, tid, | ||
| transaction.description, transaction.currency, | ||
| replace_var("TID", purchase.return_url, tid), | ||
| get_best_lang(identity.user), purchase.mobile) | ||
| except PaymentServiceException as pse: | ||
| logger.error("Failed to initialize payment: %s", pse) | ||
| raise pse | ||
| pt_expire = payment_transaction.get_expire() | ||
| if pt_expire is not None: | ||
| if transaction.expire is None or pt_expire < transaction.expire: | ||
| transaction.expire = pt_expire | ||
| transaction.payment_method_transaction_data = payment_transaction.save_dict() | ||
| elif purchase.use_purse: | ||
| abort(400, "Not enough funds available in purse for purchase") | ||
| else: | ||
| abort(400, "No payment method specified") | ||
| webview_url = payment_transaction.get_webview_url() | ||
| else: | ||
| transaction.payment_method_id = None | ||
| transaction.payment_method_transaction_data = None | ||
| transaction.payment_method_amount = None | ||
| webview_url = None | ||
| transaction.wallet = wallet | ||
| transaction.state = TransactionState.USER_INTERACTION_PENDING if webview_url is not None \ | ||
| else TransactionState.FINALIZE_PENDING | ||
| transaction.save() | ||
| return webview_url |
There was a problem hiding this comment.
✅ No longer an issue: Deep, Nested Complexity
_initialize_traveller_purchase is no longer above the threshold for nested complexity depth
Comment on lines
-341
to
-352
| def _mark_purchased(transaction: DbTransaction) -> None: | ||
| """Mark products and transaction as purchased""" | ||
| transaction.state = TransactionState.PURCHASED | ||
| for item in transaction.items: | ||
| if item.mtb_product_ids: | ||
| for mp_id in item.mtb_product_ids: | ||
| try: | ||
| mtb_prod = get_db_mtb_product(None, mp_id, all=True, refresh=False) | ||
| mtb_prod.purchased = True | ||
| mtb_prod.save() | ||
| except Exception as exc: | ||
| logger.error("Failed to mark mtb_product {mp_id} as purchase", exc_info=exc) |
There was a problem hiding this comment.
✅ No longer an issue: Deep, Nested Complexity
_mark_purchased is no longer above the threshold for nested complexity depth
Comment on lines
-421
to
-450
| def cancel_payment_transaction(auth_ctx: AuthorizationContext, transaction: DbTransaction) -> None: | ||
| """Cancel transaction and refund payments and invalidate products""" | ||
| for item in transaction.items: | ||
| if item.mtb_product_ids: | ||
| for mp_id in item.mtb_product_ids: | ||
| try: | ||
| prod = get_db_mtb_product(auth_ctx, mp_id) | ||
| check_mtb_product_useable(auth_ctx, prod) | ||
| except AbortException: | ||
| abort(409, "Transaction contains lent products") | ||
| state = transaction.state | ||
| if state == TransactionState.PURCHASED or state == TransactionState.FINALIZE_PENDING \ | ||
| or state == TransactionState.USER_INTERACTION_PENDING: | ||
| _refund_payment(transaction, "Cancel requested") | ||
| _refund_purse(transaction) | ||
| else: | ||
| abort(501, f"Transaction in state {state} isn't cancellable") | ||
|
|
||
| for item in transaction.items: | ||
| if item.mtb_product_ids: | ||
| for mp_id in item.mtb_product_ids: | ||
| try: | ||
| cancel_mtb_product(auth_ctx, mp_id, transaction) | ||
| except Exception as exc: | ||
| logger.error("Failed to cancel mtb_product {mp_id}", exc_info=exc) | ||
| # TODO remove product from traveller | ||
| transaction.cancellable = False | ||
| transaction.cancellable_expire = None | ||
| transaction.state = TransactionState.CANCELLED | ||
| transaction.save() |
There was a problem hiding this comment.
✅ No longer an issue: Deep, Nested Complexity
cancel_payment_transaction is no longer above the threshold for nested complexity depth
Comment on lines
-61
to
-94
| def get_transactions(identity: Identity, after: str = None, before: str = None, mtb_product_id: str = None, | ||
| traveller_id: str = None, vendor_id: str = None, wallet_id: str = None) -> List[Transaction]: | ||
| """Get all transactions matching filter""" | ||
| query = None | ||
| if after: | ||
| query = Q(timestamp__gte=after) | ||
| if before: | ||
| if query is None: | ||
| query = Q(timestamp__lt=before) | ||
| else: | ||
| query = query & Q(timestamp__lt=before) | ||
| if mtb_product_id: | ||
| if query is None: | ||
| query = Q(items__mtb_product_ids=mtb_product_id) | ||
| else: | ||
| query = query & Q(items__mtb_product_ids=mtb_product_id) | ||
| if traveller_id: | ||
| q = Q(owner=db_id(traveller_id)) | Q(recipient=db_id(traveller_id)) | ||
| if query is None: | ||
| query = q | ||
| else: | ||
| query &= q | ||
| if vendor_id: | ||
| q = Q(owner=db_id(vendor_id)) | Q(recipient=db_id(vendor_id)) | ||
| if query is None: | ||
| query = q | ||
| else: | ||
| query &= q | ||
| if wallet_id: | ||
| if query is None: | ||
| query = Q(wallet=db_id(wallet_id)) | ||
| else: | ||
| query = query & Q(wallet=db_id(wallet_id)) | ||
| return [to_transaction_model(_update_cancellable(trans)) for trans in (DbTransaction.objects(q_obj=query))] |
There was a problem hiding this comment.
✅ No longer an issue: Excess Number of Function Arguments
get_transactions is no longer above the threshold for number of arguments
There was a problem hiding this comment.
✅ Code Health Improved (1 files improve in Code Health)
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| python-example.py | 8.28 → 10.00 | Low Cohesion, Complex Method, Complex Conditional, Bumpy Road Ahead, Deep, Nested Complexity, Excess Number of Function Arguments |
Quality Gate Profile: Custom Configuration
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
adamtornhill
force-pushed
the
pr-code-improvement
branch
from
January 16, 2025 13:07
b94bd9e to
e60a8ac
Compare
There was a problem hiding this comment.
✅ Code Health Improved (1 files improve in Code Health)
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| python-example.py | 8.28 → 10.00 | Low Cohesion, Complex Method, Complex Conditional, Bumpy Road Ahead, Deep, Nested Complexity, Excess Number of Function Arguments |
Quality Gate Profile:
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
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
1 participant
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.
No description provided.