From 289d417cc93eafdb891873f047f8551c73bf7117 Mon Sep 17 00:00:00 2001 From: Rechner Fox <659028+rechner@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:39:42 -0700 Subject: [PATCH] Fix low-hanging SonarQube findings --- memberportal/access/admin.py | 11 +- memberportal/api_access/consumers.py | 6 +- memberportal/api_access/views.py | 36 +++---- memberportal/api_admin_tools/admin.py | 2 +- memberportal/api_billing/views.py | 109 ++++++++++++-------- memberportal/api_general/views.py | 136 ++++++++++++++----------- memberportal/api_meeting/admin.py | 2 +- memberportal/api_member_tools/views.py | 25 ++--- memberportal/api_metrics/admin.py | 2 +- memberportal/api_metrics/tasks.py | 13 ++- memberportal/profile/models.py | 35 ++++--- 11 files changed, 226 insertions(+), 151 deletions(-) diff --git a/memberportal/access/admin.py b/memberportal/access/admin.py index b502a9af..13ffc4d7 100644 --- a/memberportal/access/admin.py +++ b/memberportal/access/admin.py @@ -1,6 +1,15 @@ from django.contrib import admin from rest_framework_api_key.admin import APIKeyModelAdmin -from .models import * +from .models import ( + AccessControlledDevice, + AccessControlledDeviceAPIKey, + ExternalAccessControlAPIKey, + Doors, + DoorLog, + Interlock, + InterlockLog, + MemberbucksDevice, +) @admin.register(AccessControlledDeviceAPIKey) diff --git a/memberportal/api_access/consumers.py b/memberportal/api_access/consumers.py index 034b8125..c19675be 100644 --- a/memberportal/api_access/consumers.py +++ b/memberportal/api_access/consumers.py @@ -157,7 +157,7 @@ def receive_json(self, content=None, **kwargs): ) except Exception as e: - logger.error("Error receiving message from device: %s", e) + logger.exception("Error receiving message from device") self.send_json({"command": "error"}) raise e @@ -544,7 +544,7 @@ def handle_other_packet(self, content): ) message = f"We just tried to debit ${amount} from your {config.MEMBERBUCKS_NAME} balance but were not " f"successful. You currently have ${profile.memberbucks_balance}. If this wasn't you, please let us know " - f"immediately." + "immediately." User.objects.get(profile=profile).email_notification(subject, message) @@ -613,7 +613,7 @@ def handle_other_packet(self, content): ) message = f"Description: {transaction.description}. Balance Remaining: " f"${profile.memberbucks_balance}. If this wasn't you, or you believe there " - f"has been an error, please let us know." + "has been an error, please let us know." User.objects.get(profile=profile).email_notification(subject, message) diff --git a/memberportal/api_access/views.py b/memberportal/api_access/views.py index a45e991b..c2b1b4f4 100644 --- a/memberportal/api_access/views.py +++ b/memberportal/api_access/views.py @@ -12,6 +12,8 @@ from rest_framework.views import APIView from constance import config +API_DISABLED_MESSAGE = "This API is disabled in the config." + class AccessSystemStatus(APIView): """ @@ -21,7 +23,7 @@ class AccessSystemStatus(APIView): permission_classes = (HasExternalAccessControlAPIKey | permissions.IsAdminUser,) def get(self, request): - statusObject = { + status_object = { "doors": [], "interlocks": [], "memberbucksDevices": [], @@ -58,7 +60,7 @@ def report_count(device_type: str): offline = door.get_unavailable() update_count(offline, door.locked_out) - statusObject["doors"].append( + status_object["doors"].append( { "id": door.id, "name": door.name, @@ -78,7 +80,7 @@ def report_count(device_type: str): offline = interlock.get_unavailable() update_count(offline, interlock.locked_out) - statusObject["interlocks"].append( + status_object["interlocks"].append( { "id": interlock.id, "name": interlock.name, @@ -94,20 +96,20 @@ def report_count(device_type: str): report_count("interlock") reset_count() - for memberbucksDevice in MemberbucksDevice.objects.all(): - offline = memberbucksDevice.get_unavailable() - update_count(offline, memberbucksDevice.locked_out) + for memberbucks_device in MemberbucksDevice.objects.all(): + offline = memberbucks_device.get_unavailable() + update_count(offline, memberbucks_device.locked_out) - statusObject["memberbucksDevices"].append( + status_object["memberbucksDevices"].append( { - "id": memberbucksDevice.id, - "name": memberbucksDevice.name, - "lastSeen": memberbucksDevice.last_seen, - "lockedOut": memberbucksDevice.locked_out, + "id": memberbucks_device.id, + "name": memberbucks_device.name, + "lastSeen": memberbucks_device.last_seen, + "lockedOut": memberbucks_device.locked_out, "offline": offline, } ) - if offline and memberbucksDevice.report_online_status: + if offline and memberbucks_device.report_online_status: a_device_is_offline = True # report spacebucksDevices metrics @@ -115,9 +117,9 @@ def report_count(device_type: str): reset_count() if error_if_offline and a_device_is_offline: - return Response(statusObject, status=status.HTTP_503_SERVICE_UNAVAILABLE) + return Response(status_object, status=status.HTTP_503_SERVICE_UNAVAILABLE) - return Response(statusObject) + return Response(status_object) class UserAccessPermissions(APIView): @@ -260,7 +262,7 @@ def post(self, request, door_id): return Response({"success": bumped}) else: return Response( - {"success": False, "error": "This API is disabled in the config."}, + {"success": False, "error": API_DISABLED_MESSAGE}, status=status.HTTP_403_FORBIDDEN, ) @@ -286,7 +288,7 @@ def post(self, request, door_id=None, interlock_id=None): return Response({"success": locked}) else: return Response( - {"success": False, "error": "This API is disabled in the config."}, + {"success": False, "error": API_DISABLED_MESSAGE}, status=status.HTTP_403_FORBIDDEN, ) @@ -312,6 +314,6 @@ def post(self, request, door_id=None, interlock_id=None): return Response({"success": unlocked}) else: return Response( - {"success": False, "error": "This API is disabled in the config."}, + {"success": False, "error": API_DISABLED_MESSAGE}, status=status.HTTP_403_FORBIDDEN, ) diff --git a/memberportal/api_admin_tools/admin.py b/memberportal/api_admin_tools/admin.py index a85f7856..0ffc3e21 100644 --- a/memberportal/api_admin_tools/admin.py +++ b/memberportal/api_admin_tools/admin.py @@ -1,5 +1,5 @@ from django.contrib import admin -from .models import * +from .models import MemberTier, PaymentPlan @admin.register(MemberTier) diff --git a/memberportal/api_billing/views.py b/memberportal/api_billing/views.py index 5b4950d7..3a0d128b 100644 --- a/memberportal/api_billing/views.py +++ b/memberportal/api_billing/views.py @@ -7,8 +7,11 @@ CompleteSignupResult, SignupTriggeredBy, CancelTriggeredBy, + ProfileState, + SubscriptionState, ) -from api_admin_tools.models import * + +# from api_admin_tools.models import from .models import ProcessedStripeEvent from rest_framework import status, permissions @@ -18,6 +21,7 @@ import stripe import logging import uuid +from enum import Enum from services.induction import refresh as refresh_induction from services.emails import send_email_to_admin from constance import config @@ -29,6 +33,24 @@ logger = logging.getLogger("billing") +BILLING_STRIPE_ERROR = "billing.stripeError" +BILLING_STATE_LOCKED = "billing.stateLocked" +BILLING_INVOICING_DISABLED = "billing.invoiceDisabled" +BILLING_NEW_SUBSCRIPTIONS_DISABLED = "billing.newSubscriptionsDisabled" +SIGNUP_SUBSCRIPTION_FAILED = "signup.subscriptionFailed" +SIGNUP_AWAITING_INVOICE_PAYMENT = "signup.awaitingInvoicePayment" +SIGNUP_REQUIREMENTS_NOT_MET = "signup.requirementsNotMet" +SIGNUP_SKIP_NOT_ALLOWED = "signup.skipNotAllowed" +ACCESS_CARD_MEMBER_ENTRY_DISABLED = "accessCard.memberEntryDisabled" +ACCESS_CARD_REQUIRED = "accessCard.required" +ACCESS_CARD_ADMIN_REBIND_REQUIRED = "accessCard.adminRebindRequired" +ACCESS_CARD_ALREADY_BOUND = "accessCard.alreadyBound" +ACCESS_CARD_ALREAD_IN_USE = "accessCard.alreadyInUse" +PAYMENT_PLAN_DOES_NOT_EXIST = "paymentPlan.notExists" + +INVOICE_PAID = "invoice.paid" +INVOICE_PAYMENT_FAILED = "invoice.payment_failed" + def _get_subscription_current_period_end(subscription): """Return the current billing-period end from a Stripe subscription.""" @@ -76,7 +98,7 @@ def ensure_stripe_customer(user): except stripe.error.StripeError as e: capture_exception(e) user.log_event("Error while creating stripe customer.", "stripe", str(e)) - return False, "billing.stripeError" + return False, BILLING_STRIPE_ERROR class StripeAPIView(APIView): @@ -115,7 +137,7 @@ def get(self, request): "Stripe error while creating SetupIntent.", "stripe", str(e) ) return Response( - {"success": False, "message": "billing.stripeError"}, + {"success": False, "message": BILLING_STRIPE_ERROR}, status=status.HTTP_503_SERVICE_UNAVAILABLE, ) @@ -150,7 +172,7 @@ def post(self, request): str(e), ) return Response( - {"success": False, "message": "billing.stripeError"}, + {"success": False, "message": BILLING_STRIPE_ERROR}, status=status.HTTP_503_SERVICE_UNAVAILABLE, ) @@ -215,7 +237,7 @@ def delete(self, request): str(e), ) return Response( - {"success": False, "message": "billing.stripeError"}, + {"success": False, "message": BILLING_STRIPE_ERROR}, status=status.HTTP_503_SERVICE_UNAVAILABLE, ) @@ -424,7 +446,7 @@ def post(self, request, plan_id): # Refuse before any Stripe call so a locked member can't pay into a void. if request.user.profile.state_locked: return Response( - {"success": False, "message": "billing.stateLocked"}, + {"success": False, "message": BILLING_STATE_LOCKED}, status=status.HTTP_403_FORBIDDEN, ) @@ -433,7 +455,7 @@ def post(self, request, plan_id): # PaymentPlanResume for cancelling members must all keep working. if not config.ENABLE_NEW_SUBSCRIPTIONS: return Response( - {"success": False, "message": "billing.newSubscriptionsDisabled"}, + {"success": False, "message": BILLING_NEW_SUBSCRIPTIONS_DISABLED}, status=status.HTTP_503_SERVICE_UNAVAILABLE, ) @@ -445,7 +467,7 @@ def post(self, request, plan_id): if billing_method == "invoice" and not config.ENABLE_INVOICE_BILLING: return Response( - {"success": False, "message": "billing.invoiceDisabled"}, + {"success": False, "message": BILLING_INVOICING_DISABLED}, status=status.HTTP_400_BAD_REQUEST, ) @@ -470,7 +492,7 @@ def post(self, request, plan_id): # locked member. if locked_profile.state_locked: return Response( - {"success": False, "message": "billing.stateLocked"}, + {"success": False, "message": BILLING_STATE_LOCKED}, status=status.HTTP_403_FORBIDDEN, ) @@ -483,11 +505,13 @@ def post(self, request, plan_id): if error_response is not None: return error_response - if new_subscription.status == "active": + if new_subscription.status == SubcriptionState.ACTIVE: locked_profile.stripe_subscription_id = new_subscription.id locked_profile.membership_plan = new_plan locked_profile.subscription_status = ( - "pending" if billing_method == "invoice" else "active" + SubscriptionState.PENDING + if billing_method == "invoice" + else SubscriptionState.INACTIVE ) locked_profile.billing_method = billing_method locked_profile.pending_signup_email_sent = False @@ -507,7 +531,7 @@ def post(self, request, plan_id): "", ) - if new_subscription.status == "active": + if new_subscription.status == SubscriptionState.ACTIVE: # Outside the atomic so complete_signup can take its own lock. locked_profile.complete_signup(SignupTriggeredBy.SUBSCRIPTION_CREATED) return Response({"success": True}) @@ -522,7 +546,7 @@ def post(self, request, plan_id): # doesn't dangle on the customer and trigger a duplicate next try. _cancel_failed_subscription(request.user, new_subscription.id) - return Response({"success": False, "message": "signup.subscriptionFailed"}) + return Response({"success": False, "message": SIGNUP_SUBSCRIPTION_FAILED}) class CanSignup(APIView): @@ -554,14 +578,14 @@ class AssignAccessCard(APIView): def post(self, request): if not config.MEMBER_CAN_ENTER_ACCESS_CARD: return Response( - {"success": False, "message": "accessCard.memberEntryDisabled"}, + {"success": False, "message": ACCESS_CARD_MEMBER_ENTRY_DISABLED}, status=status.HTTP_403_FORBIDDEN, ) access_card = (request.data.get("accessCard") or "").strip() if not access_card: return Response( - {"success": False, "message": "accessCard.required"}, + {"success": False, "message": ACCESS_CARD_REQUIRED}, status=status.HTTP_400_BAD_REQUEST, ) @@ -575,13 +599,16 @@ def post(self, request): pk=request.user.profile.pk ) - if locked_profile.state not in ("noob", "accountonly"): + if locked_profile.state not in ( + ProfileState.NOOB, + ProfileState.ACCOUNT_ONLY, + ): request.user.log_event( f"Member tried to self-rebind RFID while state={locked_profile.state}; refused.", "profile", ) return Response( - {"success": False, "message": "accessCard.adminRebindRequired"}, + {"success": False, "message": ACCESS_CARD_ADMIN_REBIND_REQUIRED}, status=status.HTTP_403_FORBIDDEN, ) @@ -591,7 +618,7 @@ def post(self, request): "profile", ) return Response( - {"success": False, "message": "accessCard.alreadyBound"}, + {"success": False, "message": ACCESS_CARD_ALREADY_BOUND}, status=status.HTTP_409_CONFLICT, ) @@ -605,7 +632,7 @@ def post(self, request): "profile", ) return Response( - {"success": False, "message": "accessCard.alreadyInUse"}, + {"success": False, "message": ACCESS_CARD_ALREAD_IN_USE}, status=status.HTTP_409_CONFLICT, ) @@ -623,7 +650,7 @@ def post(self, request): "profile", ) return Response( - {"success": False, "message": "accessCard.alreadyInUse"}, + {"success": False, "message": ACCESS_CARD_ALREAD_IN_USE}, status=status.HTTP_409_CONFLICT, ) @@ -666,27 +693,27 @@ def _serialize_complete_signup(result: CompleteSignupResult) -> Response: { "success": True, "awaitingPayment": True, - "message": "signup.awaitingInvoicePayment", + "message": SIGNUP_AWAITING_INVOICE_PAYMENT, } ) if result.outcome == CompleteSignupOutcome.REQUIREMENTS_UNMET: return Response( { "success": False, - "message": "signup.requirementsNotMet", + "message": SIGNUP_REQUIREMENTS_NOT_MET, "items": result.required_steps, } ) if result.outcome == CompleteSignupOutcome.STATE_LOCKED: return Response( - {"success": False, "message": "billing.stateLocked"}, + {"success": False, "message": BILLING_STATE_LOCKED}, status=status.HTTP_403_FORBIDDEN, ) # NO_SUBSCRIPTION return Response( { "success": False, - "message": "signup.requirementsNotMet", + "message": SIGNUP_REQUIREMENTS_NOT_MET, "items": ["No active subscription found."], } ) @@ -721,18 +748,18 @@ def post(self, request): ) if ( - locked_profile.state != "noob" - or locked_profile.subscription_status != "inactive" + locked_profile.state != ProfileState.NOOB + or locked_profile.subscription_status != SubscriptionStatus.INACTIVE ): return Response( { "success": False, - "message": "signup.skipNotAllowed", + "message": SIGNUP_SKIP_NOT_ALLOWED, }, status=status.HTTP_409_CONFLICT, ) - locked_profile.state = "accountonly" + locked_profile.state = ProfileState.ACCOUNT_ONLY locked_profile.save(update_fields=["state"]) return Response({"success": True}) @@ -784,7 +811,7 @@ def get(self, request): def _no_plan_response(user): user.log_event("Member tried to modify nonexistant membership plan.", "stripe") return Response( - {"success": False, "message": "paymentPlan.notExists"}, + {"success": False, "message": PAYMENT_PLAN_DOES_NOT_EXIST}, status=status.HTTP_404_NOT_FOUND, ) @@ -894,7 +921,7 @@ class PaymentPlanResume(StripeAPIView): def post(self, request): if request.user.profile.state_locked: return Response( - {"success": False, "message": "billing.stateLocked"}, + {"success": False, "message": BILLING_STATE_LOCKED}, status=status.HTTP_403_FORBIDDEN, ) @@ -934,7 +961,7 @@ def _resume_by_recreating(self, request, current_plan): # the orphan-Stripe-sub rationale. if locked_profile.state_locked: return Response( - {"success": False, "message": "billing.stateLocked"}, + {"success": False, "message": BILLING_STATE_LOCKED}, status=status.HTTP_403_FORBIDDEN, ) @@ -950,10 +977,12 @@ def _resume_by_recreating(self, request, current_plan): if error_response is not None: return error_response - if new_subscription.status == "active": + if new_subscription.status == SubscriptionState.ACTIVE: locked_profile.stripe_subscription_id = new_subscription.id locked_profile.subscription_status = ( - "pending" if billing_method == "invoice" else "active" + SubscriptionState.PENDING + if billing_method == "invoice" + else SubscriptionState.ACTIVE ) locked_profile.pending_signup_email_sent = False locked_profile.save( @@ -984,7 +1013,7 @@ def _resume_by_recreating(self, request, current_plan): # Cancel the non-active sub so a retry doesn't duplicate it. _cancel_failed_subscription(request.user, new_subscription.id) - return Response({"success": False, "message": "signup.subscriptionFailed"}) + return Response({"success": False, "message": SIGNUP_SUBSCRIPTION_FAILED}) def _resume_cancelling(self, request): # Lock so a concurrent webhook can't null stripe_subscription_id @@ -1003,7 +1032,7 @@ def _resume_cancelling(self, request): or locked_profile.subscription_status != "cancelling" ): return Response( - {"success": False, "message": "paymentPlan.notExists"}, + {"success": False, "message": PAYMENT_PLAN_DOES_NOT_EXIST}, status=status.HTTP_409_CONFLICT, ) @@ -1104,7 +1133,7 @@ def _cancel_pending(self, request): or not locked_profile.stripe_subscription_id ): return Response( - {"success": False, "message": "paymentPlan.notExists"}, + {"success": False, "message": PAYMENT_PLAN_DOES_NOT_EXIST}, status=status.HTTP_409_CONFLICT, ) @@ -1243,7 +1272,7 @@ def _cancel_active(self, request): if not locked_profile.stripe_subscription_id: return Response( - {"success": False, "message": "paymentPlan.notExists"}, + {"success": False, "message": PAYMENT_PLAN_DOES_NOT_EXIST}, status=status.HTTP_409_CONFLICT, ) @@ -1370,7 +1399,7 @@ def post(self, request): payload=request.body, sig_header=signature, secret=webhook_secret ) except Exception as e: - logger.error(e) + logger.exception("Error validating Stripe signature.") capture_exception(e) return Response({"error": "Error validating Stripe signature."}) @@ -1407,7 +1436,7 @@ def post(self, request): # memberbucks-related charges, etc.) and acting on those would falsely # activate the member or send misleading "membership payment failed" # emails. The admin "mark paid out-of-band" tool has the same guard. - if event_type in ("invoice.paid", "invoice.payment_failed"): + if event_type in (INVOICE_PAID, INVOICE_PAYMENT_FAILED): invoice_subscription = data.get("subscription") if ( not invoice_subscription @@ -1421,7 +1450,7 @@ def post(self, request): # check BEFORE the dedup insert so an out-of-scope event doesn't # poison its own retries — fix it, redeliver, and processing # picks up cleanly. - if event_type in ("invoice.paid", "invoice.payment_failed"): + if event_type in (INVOICE_PAID, INVOICE_PAYMENT_FAILED): invoice_subscription = _invoice_subscription_id(data) if ( not invoice_subscription diff --git a/memberportal/api_general/views.py b/memberportal/api_general/views.py index cf1b6197..f395dd0b 100644 --- a/memberportal/api_general/views.py +++ b/memberportal/api_general/views.py @@ -13,7 +13,7 @@ from django.db import transaction, IntegrityError from django.utils import timezone import datetime -from profile.models import User, Profile, queue_listmonk_member_sync +from profile.models import User, Profile, ProfileState, queue_listmonk_member_sync from profile.phone import to_e164 from rest_framework import status, permissions, generics, serializers @@ -33,6 +33,31 @@ logger = logging.getLogger("general") +ERROR_ACCOUNT_ALREADY_EXISTS = "error.accountAlreadyExists" +ERROR_SCREEN_NAME_ALREADY_EXISTS = "error.screenNameAlreadyExists" + +ERROR_PASSWORD_INVALID = "error.passwordInvalid" +ERROR_PASSWORD_TOO_SHORT = "error.passwordTooShort" +ERROR_PASSWORD_TOO_LONG = "error.passwordTooLong" +ERROR_PASSWORD_TOO_COMMON = "error.passwordTooCommon" +ERROR_PASSWORD_TOO_SIMILAR = "error.passwordTooSimilar" +ERROR_PASSWORD_ENTIRELY_NUMERIC = "error.passwordEntirelyNumeric" +ERROR_PASSWORD_COMPROMISED = "error.passwordCompromised" +ERROR_FIELD_REQUIRED = "error.fieldRequired" +ERROR_EMAIL_TOO_LONG = "error.emailTooLong" +ERROR_FIRST_NAME_TOO_LONG = "error.firstNameTooLong" +ERROR_LAST_NAME_TOO_LONG = "error.lastNameTooLong" +ERROR_SCREEN_NAME_REQUIRED = "error.screenNameRequired" +ERROR_SCREEN_NAME_TOO_LONG = "error.screenNameTooLong" +ERROR_MOBILE_TOO_LONG = "error.mobileTooLong" +ERROR_VEHICLE_PLATE_TOO_LONG = "error.vehiclePlateTooLong" +ERROR_REGISTRATION_CLOSED = "error.registrationClosed" +ERROR_EMAIL_NOT_VERIFIED = "error.emailNotVerified" +ERROR_EMAIL_VERIFICATION_FAILED = "error.emailVerificationFailed" +ERROR_EMAIL_VERIFICATION_EXPIRED = "error.emailVerificationExpired" +VALIDATION_INVALID_EMAIL = "validation.invalidEmail" +VALIDATION_INVALID_PHONE = "validation.invalidPhone" + def _parse_terms_acceptance_cards(): try: @@ -112,7 +137,7 @@ def get(self, request): try: homepage_cards = json.loads(config.HOME_PAGE_CARDS) - except: + except json.JSONDecodeError: homepage_cards = [ { "title": "Error loading configuration", @@ -125,7 +150,7 @@ def get(self, request): try: webcam_links = json.loads(config.WEBCAM_PAGE_URLS) - except: + except json.JSONDecodeError: webcam_links = [ ["Error Loading Webcam Configuration", ""], ] @@ -331,7 +356,7 @@ def post(self, request): if not user.email_verified: return Response( - {"message": "error.emailNotVerified"}, status=status.HTTP_403_FORBIDDEN + {"message": ERROR_EMAIL_NOT_VERIFIED}, status=status.HTTP_403_FORBIDDEN ) # rfid matches a user so log them in @@ -504,7 +529,7 @@ def get(self, request): "membershipTier": ( p.membership_plan.member_tier.get_object() if p.membership_plan - else None if p.membership_plan else None + else None ), "subscriptionState": p.subscription_status, "billingMethod": p.billing_method, @@ -518,8 +543,8 @@ def get(self, request): # assuming here that the zeroth party will always be the member for docs in submission["submitters"][0]["documents"]: response["memberdocsLink"].append(docs["url"]) - except: - pass + except KeyError as e: + capture_exception(e) # Induction links and banner state now derive from independent local # provider checks. Profile reads never query external providers or @@ -559,7 +584,7 @@ def put(self, request): .exists() ): return Response( - {"message": "error.accountAlreadyExists"}, + {"message": ERROR_ACCOUNT_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) @@ -571,7 +596,7 @@ def put(self, request): .exists() ): return Response( - {"message": "error.screenNameAlreadyExists"}, + {"message": ERROR_SCREEN_NAME_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) @@ -585,7 +610,7 @@ def put(self, request): phone = to_e164(phone, config.PROFILE_DEFAULT_PHONE_REGION) except ValueError: return Response( - {"message": "validation.invalidPhone"}, + {"message": VALIDATION_INVALID_PHONE}, status=status.HTTP_400_BAD_REQUEST, ) @@ -614,7 +639,7 @@ def put(self, request): # stale full-row save. Profile.save() rides `modified` # along automatically. p.save(update_fields=profile_fields) - if p.state in ("active", "inactive"): + if p.state in (ProfileState.ACTIVE, ProfileState.INACTIVE): queue_listmonk_member_sync(p, p.state) except IntegrityError: # Race with a concurrent register/update: pre-checks passed @@ -626,11 +651,11 @@ def put(self, request): .exists() ): return Response( - {"message": "error.accountAlreadyExists"}, + {"message": ERROR_ACCOUNT_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) return Response( - {"message": "error.screenNameAlreadyExists"}, + {"message": ERROR_SCREEN_NAME_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) @@ -818,16 +843,15 @@ def get(self, request): # Maps Django password-validator error codes (AUTH_PASSWORD_VALIDATORS) # to frontend i18n keys; unknown codes fall back to error.passwordInvalid. + PASSWORD_VALIDATION_ERROR_KEYS = { - "password_too_short": "error.passwordTooShort", - "password_too_common": "error.passwordTooCommon", - "password_entirely_numeric": "error.passwordEntirelyNumeric", - "password_too_similar": "error.passwordTooSimilar", - "password_compromised": "error.passwordCompromised", + "password_too_short": ERROR_PASSWORD_TOO_SHORT, + "password_too_common": ERROR_PASSWORD_TOO_COMMON, + "password_entirely_numeric": ERROR_PASSWORD_ENTIRELY_NUMERIC, + "password_too_similar": ERROR_PASSWORD_TOO_SIMILAR, + "password_compromised": ERROR_PASSWORD_COMPROMISED, } -REQUIRE_MOBILE = True # TODO: migrate to a constance flag - class RegisterSerializer(serializers.Serializer): # Every error message is an i18n key resolved by the frontend, not @@ -837,11 +861,11 @@ class RegisterSerializer(serializers.Serializer): required=True, max_length=255, error_messages={ - "required": "error.fieldRequired", - "null": "error.fieldRequired", - "blank": "error.fieldRequired", - "invalid": "validation.invalidEmail", - "max_length": "error.emailTooLong", + "required": ERROR_FIELD_REQUIRED, + "null": ERROR_FIELD_REQUIRED, + "blank": ERROR_FIELD_REQUIRED, + "invalid": VALIDATION_INVALID_EMAIL, + "max_length": ERROR_EMAIL_TOO_LONG, }, ) password = serializers.CharField( @@ -850,11 +874,11 @@ class RegisterSerializer(serializers.Serializer): min_length=8, max_length=128, error_messages={ - "required": "error.fieldRequired", - "null": "error.fieldRequired", - "blank": "error.fieldRequired", - "min_length": "error.passwordTooShort", - "max_length": "error.passwordTooLong", + "required": ERROR_FIELD_REQUIRED, + "null": ERROR_FIELD_REQUIRED, + "blank": ERROR_FIELD_REQUIRED, + "min_length": ERROR_PASSWORD_TOO_SHORT, + "max_length": ERROR_PASSWORD_TOO_LONG, }, ) firstName = serializers.CharField( @@ -862,10 +886,10 @@ class RegisterSerializer(serializers.Serializer): max_length=30, allow_blank=False, error_messages={ - "required": "error.fieldRequired", - "null": "error.fieldRequired", - "blank": "error.fieldRequired", - "max_length": "error.firstNameTooLong", + "required": ERROR_FIELD_REQUIRED, + "null": ERROR_FIELD_REQUIRED, + "blank": ERROR_FIELD_REQUIRED, + "max_length": ERROR_FIRST_NAME_TOO_LONG, }, ) lastName = serializers.CharField( @@ -873,10 +897,10 @@ class RegisterSerializer(serializers.Serializer): max_length=30, allow_blank=False, error_messages={ - "required": "error.fieldRequired", - "null": "error.fieldRequired", - "blank": "error.fieldRequired", - "max_length": "error.lastNameTooLong", + "required": ERROR_FIELD_REQUIRED, + "null": ERROR_FIELD_REQUIRED, + "blank": ERROR_FIELD_REQUIRED, + "max_length": ERROR_LAST_NAME_TOO_LONG, }, ) screenName = serializers.CharField( @@ -885,7 +909,7 @@ class RegisterSerializer(serializers.Serializer): allow_blank=True, allow_null=True, default=None, - error_messages={"max_length": "error.screenNameTooLong"}, + error_messages={"max_length": ERROR_SCREEN_NAME_TOO_LONG}, ) # allow_null: the form posts null for fields it isn't collecting; # validate() normalises that to "". @@ -895,7 +919,7 @@ class RegisterSerializer(serializers.Serializer): allow_blank=True, allow_null=True, default="", - error_messages={"max_length": "error.mobileTooLong"}, + error_messages={"max_length": ERROR_MOBILE_TOO_LONG}, ) vehicleRegistrationPlate = serializers.CharField( required=False, @@ -903,7 +927,7 @@ class RegisterSerializer(serializers.Serializer): allow_blank=True, allow_null=True, default="", - error_messages={"max_length": "error.vehiclePlateTooLong"}, + error_messages={"max_length": ERROR_VEHICLE_PLATE_TOO_LONG}, ) def validate_email(self, value): @@ -925,8 +949,8 @@ def validate(self, attrs): else "" ) - if REQUIRE_MOBILE and config.COLLECT_PHONE_NUMBER and not attrs["mobile"]: - raise serializers.ValidationError({"mobile": "error.fieldRequired"}) + if config.COLLECT_PHONE_NUMBER and not attrs["mobile"]: + raise serializers.ValidationError({"mobile": ERROR_FIELD_REQUIRED}) # Store the phone number in E.164 format. if attrs["mobile"]: @@ -935,11 +959,11 @@ def validate(self, attrs): attrs["mobile"], config.PROFILE_DEFAULT_PHONE_REGION ) except ValueError: - raise serializers.ValidationError({"mobile": "validation.invalidPhone"}) + raise serializers.ValidationError({"mobile": VALIDATION_INVALID_PHONE}) if not attrs.get("screenName") and config.REQUIRE_SCREEN_NAME: raise serializers.ValidationError( - {"screenName": "error.screenNameRequired"} + {"screenName": ERROR_SCREEN_NAME_REQUIRED} ) # Run Django's AUTH_PASSWORD_VALIDATORS — min-length is already @@ -960,9 +984,7 @@ def validate(self, attrs): # (a pwned + common password trips two validators). keys = list( dict.fromkeys( - PASSWORD_VALIDATION_ERROR_KEYS.get( - err.code, "error.passwordInvalid" - ) + PASSWORD_VALIDATION_ERROR_KEYS.get(err.code, ERROR_PASSWORD_INVALID) for err in e.error_list ) ) @@ -1063,7 +1085,7 @@ def _subscribe(): ) except Exception as e: sentry_sdk.capture_exception(e) - logger.error(e) + logger.exception(e) transaction.on_commit(_subscribe) @@ -1086,7 +1108,7 @@ def post(self, request): if not config.ENABLE_REGISTRATION: return Response( { - "message": "error.registrationClosed", + "message": ERROR_REGISTRATION_CLOSED, "detail": config.REGISTRATION_DISABLED_MESSAGE, }, status=status.HTTP_503_SERVICE_UNAVAILABLE, @@ -1108,7 +1130,7 @@ def post(self, request): # the DB (Postgres email column is case-sensitive by default). if User.objects.filter(email__iexact=data["email"]).exists(): return Response( - {"message": "error.accountAlreadyExists"}, + {"message": ERROR_ACCOUNT_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) if ( @@ -1116,7 +1138,7 @@ def post(self, request): and Profile.objects.filter(screen_name__iexact=data["screenName"]).exists() ): return Response( - {"message": "error.screenNameAlreadyExists"}, + {"message": ERROR_SCREEN_NAME_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) @@ -1153,11 +1175,11 @@ def post(self, request): # which collision occurred. if User.objects.filter(email__iexact=data["email"]).exists(): return Response( - {"message": "error.accountAlreadyExists"}, + {"message": ERROR_ACCOUNT_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) return Response( - {"message": "error.screenNameAlreadyExists"}, + {"message": ERROR_SCREEN_NAME_ALREADY_EXISTS}, status=status.HTTP_409_CONFLICT, ) @@ -1178,7 +1200,7 @@ def post(self, request, verify_token): ) except (EmailVerificationToken.DoesNotExist, ValueError): return Response( - {"message": "error.emailVerificationFailed"}, + {"message": ERROR_EMAIL_VERIFICATION_FAILED}, status=status.HTTP_401_UNAUTHORIZED, ) @@ -1196,7 +1218,7 @@ def post(self, request, verify_token): ).delete() if deleted_count == 0: return Response( - {"message": "error.emailVerificationFailed"}, + {"message": ERROR_EMAIL_VERIFICATION_FAILED}, status=status.HTTP_401_UNAUTHORIZED, ) @@ -1215,6 +1237,6 @@ def post(self, request, verify_token): # verification email (see Login.post), so the explicit resend # path exists without an unauthenticated amplifier here. return Response( - {"message": "error.emailVerificationExpired"}, + {"message": ERROR_EMAIL_VERIFICATION_EXPIRED}, status=status.HTTP_403_FORBIDDEN, ) diff --git a/memberportal/api_meeting/admin.py b/memberportal/api_meeting/admin.py index e122f06f..03e0c60a 100644 --- a/memberportal/api_meeting/admin.py +++ b/memberportal/api_meeting/admin.py @@ -1,5 +1,5 @@ from django.contrib import admin -from .models import * +from .models import Meeting, ProxyVote @admin.register(Meeting) diff --git a/memberportal/api_member_tools/views.py b/memberportal/api_member_tools/views.py index 45fc057e..5ab9180d 100644 --- a/memberportal/api_member_tools/views.py +++ b/memberportal/api_member_tools/views.py @@ -1,5 +1,5 @@ from access.models import DoorLog, InterlockLog -from profile.models import Profile +from profile.models import Profile, ProfileState from api_meeting.models import Meeting from api_meeting.permissions import ProxyVotingPermission from constance import config @@ -16,6 +16,8 @@ logger = logging.getLogger("api_member_tools") +EPOCH_TIMESTAMP = "1970-01-01T00:00:00.000Z" + class SwipesList(APIView): """ @@ -89,7 +91,7 @@ def get(self, request): last_seen = [] for member in self.queryset.all(): - if not member.state == "active": + if member.state != ProfileState.ACTIVE: continue if member.last_seen is not None: @@ -201,8 +203,8 @@ def post(self, request): }, }, }, - "created": "1970-01-01T00:00:00.000Z", - "updated": "1970-01-01T00:00:00.000Z", + "created": EPOCH_TIMESTAMP, + "updated": EPOCH_TIMESTAMP, "project_id": vikunja_project_id, "bucket_id": 0, "reminder_dates": None, @@ -215,16 +217,14 @@ def post(self, request): headers={"Authorization": "Bearer " + config.VIKUNJA_API_TOKEN}, ) - if (vikunja_label_id is not None) and ( - task_response.status_code == 201 - ): + if task_response.status_code == 201: task_id = "unknown" try: task_id = task_response.json()["id"] vikunja_task_url = f"{config.VIKUNJA_API_URL}/tasks/{task_id}" label_body = { "label_id": int(vikunja_label_id), - "created": "1970-01-01T00:00:00.000Z", + "created": EPOCH_TIMESTAMP, } label_response = requests.request( @@ -246,7 +246,6 @@ def post(self, request): logger.exception( f"Failed to add label to Vikunja task {task_id}." ) - pass if task_response.status_code != 201: logger.error( @@ -333,7 +332,9 @@ class MeetingList(APIView): """ permission_classes = (ProxyVotingPermission,) - queryset = Meeting.objects.filter(date__gt=timezone.now()) + + def get_queryset(self): + return Meeting.objects.filter(date__gt=timezone.now()) def get(self, request): def get_meeting(meeting): @@ -345,7 +346,7 @@ def get_meeting(meeting): "date": date, } - response = list(map(get_meeting, self.queryset.all())) + response = list(map(get_meeting, self.get_queryset().all())) return Response(response) @@ -366,6 +367,6 @@ def get_member(member): } members = list(map(get_member, Profile.objects.filter(state="active"))) - shuffle(members) + shuffle(members) # NOSONAR python:S2245 return Response(members) diff --git a/memberportal/api_metrics/admin.py b/memberportal/api_metrics/admin.py index 139a330c..d14310b0 100644 --- a/memberportal/api_metrics/admin.py +++ b/memberportal/api_metrics/admin.py @@ -1,5 +1,5 @@ from django.contrib import admin -from api_metrics.models import * +from api_metrics.models import Metric @admin.register(Metric) diff --git a/memberportal/api_metrics/tasks.py b/memberportal/api_metrics/tasks.py index b020a9a7..5c36ff9d 100644 --- a/memberportal/api_metrics/tasks.py +++ b/memberportal/api_metrics/tasks.py @@ -1,5 +1,12 @@ from membermatters.celeryapp import app -from api_metrics.metrics import * +from api_metrics.metrics import ( + calculate_member_count, + calculate_member_count_6_months, + calculate_member_count_12_months, + calculate_subscription_count, + calculate_memberbucks_balance, + calculate_memberbucks_transactions, +) import requests from constance import config @@ -54,5 +61,5 @@ def calculate_metrics(): timeout=30, ) - except Exception as e: - logger.error(f"Failed to update Prometheus metrics: {e}") + except Exception: + logger.exception("Failed to update Prometheus metrics") diff --git a/memberportal/profile/models.py b/memberportal/profile/models.py index f370f9d3..f18e9317 100644 --- a/memberportal/profile/models.py +++ b/memberportal/profile/models.py @@ -405,21 +405,26 @@ class Meta: ] -class Profile(ExportModelOperationsMixin("profile"), models.Model): - STATES = ( - ("noob", "Needs Induction"), - ("active", "Active"), - ("inactive", "Inactive"), - ("accountonly", "Account only"), - ) +class ProfileState(models.TextChoices): + NOOB = "noob", "Noob" + ACTIVE = "active", "Active" + INACTIVE = "inactive", "Inactive" + ACCOUNT_ONLY = "accountonly", "Account Only" + + +class SubscriptionState(models.TextChoices): + INACTIVE = "inactive", "Inactive" + ACTIVE = "active", "Active" + CANCELLING = "cancelling", "Cancelling" + PENDING = "pending", "Pending" - SUBSCRIPTION_STATES = ( - ("inactive", "Inactive"), - ("active", "Active"), - ("cancelling", "Cancelling"), - ("pending", "Pending"), - ) +class BillingMethod(models.TextChoices): + CREDIT = "credit", "Credit" + INVOICE = "invoice", "Invoice" + + +class Profile(ExportModelOperationsMixin("profile"), models.Model): class Meta: permissions = [ ("change_staff", "Can change if the user is a staff member or not"), @@ -459,7 +464,7 @@ class Meta: message="Phone number must be in E.164 format, e.g. +61417123456.", ) phone = models.CharField(validators=[phone_regex], max_length=16, blank=True) - state = models.CharField(max_length=11, default="noob", choices=STATES) + state = models.CharField(max_length=11, default="noob", choices=ProfileState) vehicle_registration_plate = models.CharField(max_length=30, blank=True, null=True) membership_plan = models.ForeignKey( @@ -500,7 +505,7 @@ class Meta: max_length=100, blank=True, null=True, default="" ) subscription_status = models.CharField( - max_length=10, default="inactive", choices=SUBSCRIPTION_STATES + max_length=10, default="inactive", choices=SubscriptionState ) subscription_first_created = models.DateTimeField( default=None, blank=True, null=True, editable=False