Skip to content
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.

Sign up for GitHub

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

Provide a product group type dropdown when creating/editing them #1326

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Provide a product group type dropdown when creating/editing them #1326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c0aabb
Make ProductGroup types a ChoiceField in the form.
lukegb Jan 23, 2024
d4d9144
Fix a 500 when viewing payments in the admin panel.
lukegb Jan 23, 2024
4c3ba84
Merge branch 'main' into lukegb/validate-product-group-type
russss Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/admin/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
)
from wtforms.fields.html5 import DateField

from models.product import ProductGroup
from models.product import ProductGroup, PRODUCT_GROUP_TYPES
from models.basket import Basket

from ..common import CURRENCY_SYMBOLS
Expand Down Expand Up @@ -75,7 +75,7 @@ class ProductGroupReparentForm(Form):

class ProductGroupForm(Form):
name = StringField("Name")
type = StringField("Type")
type = SelectField("Type", choices=[(t.slug, t.name) for t in PRODUCT_GROUP_TYPES])
capacity_max = IntegerField("Maximum to sell (Optional)", [Optional()])
expires = DateField("Expiry Date (Optional)", [Optional()])

Expand Down
18 changes: 10 additions & 8 deletions models/basket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
from main import db
from . import Currency
from .exc import CapacityException
from .product import PriceTier, Voucher
from .purchase import Purchase, Ticket, AdmissionTicket
from .product import PriceTier, Voucher, PRODUCT_GROUP_TYPES_DICT
from .purchase import Purchase


class Line:
Expand Down Expand Up @@ -201,12 +201,14 @@ def create_purchases(self):
line.tier.issue_instances(issue_count)

product = line.tier.parent
if product.parent.type == "admissions":
purchase_cls = AdmissionTicket
elif product.parent.type in {"campervan", "parking"}:
purchase_cls = Ticket
else:
purchase_cls = Purchase
product_group_type = PRODUCT_GROUP_TYPES_DICT.get(
product.parent.type
)
purchase_cls = (
product_group_type.purchase_cls
if product_group_type
else Purchase
)
Comment on lines +204 to +211
Copy link
Contributor

@jayaddison jayaddison Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could combine these two, by providing a dict lookup fallback/default?

[ removed to prevent accidental commit ]

(although arguably less clear, partly due to the text wrapping to fit within line limits)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind; I see that there's an additional level of indirection for the purchase_cls lookup. Hrm. I'll ponder that a bit, but this seems OK as-is, and my suggestion's not valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the purchase class is an attribute on the thing in the dict, rather than directly being an element of it, so this doesn't work in and of itself.


price = line.tier.get_price(self.currency)
purchases = [
Expand Down
20 changes: 19 additions & 1 deletion models/product.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from __future__ import annotations
from dataclasses import dataclass
from decimal import Decimal
from collections import defaultdict
from datetime import datetime, timedelta
Expand All @@ -15,7 +16,7 @@
from main import db
from .mixins import CapacityMixin, InheritedAttributesMixin
from . import BaseModel
from .purchase import Purchase
from .purchase import Purchase, AdmissionTicket, Ticket
from .payment import Currency

if TYPE_CHECKING:
Expand Down Expand Up @@ -66,6 +67,23 @@ def one_or_none(result):
raise MultipleLoadedResultsFound()


@dataclass(frozen=True)
class ProductGroupType:
slug: str
name: str
purchase_cls: type[Purchase]


PRODUCT_GROUP_TYPES = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm muddling my way back through the design factors for the change: is there a reason these should go here instead of alongside the similar-ish constants in purchase.py?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That list doesn't seem to be used at all, so it can probably be removed.

The root issue here is that there's inevitable duplication because we have a polymorphic type hierarchy under Purchase. Which of those Purchase types is created is determined by the Product's ProductGroup.type. And the point of #1214 (I haven't checked how relevant it still is) is that we should probably only be checking one of those.

Anyway I'm happy that this PR is a net improvement to the situation.

ProductGroupType("admissions", "Admission Ticket", AdmissionTicket),
ProductGroupType("campervan", "Campervan Ticket", Ticket),
ProductGroupType("parking", "Parking", Ticket),
ProductGroupType("merchandise", "Merchandise", Purchase),
ProductGroupType("rental", "Rental", Purchase),
]
PRODUCT_GROUP_TYPES_DICT = {t.slug: t for t in PRODUCT_GROUP_TYPES}


class ProductGroup(BaseModel, CapacityMixin, InheritedAttributesMixin):
"""Represents a logical group of products.

Expand Down
2 changes: 1 addition & 1 deletion templates/admin/payments/payment.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ <h2>Payment {{ payment.id }}</h2>
{% endif %}
{% if payment.provider == 'banktransfer' and payment.state == 'inprogress' %}
<a class="btn btn-primary" href="{{ url_for('.reset_expiry', payment_id=payment.id) }}">Reset expiry</a>
<a class="btn btn-warning {% if payment.reminder_sent_at is not None -%} disabled {%- endif %}" href="{{ url_for('.send_reminder', payment_id=payment.id) }}">Remind</a>
<a class="btn btn-warning {% if payment.reminder_sent_at is not none -%} disabled {%- endif %}" href="{{ url_for('.send_reminder', payment_id=payment.id) }}">Remind</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is none was news to me, but yep, that checks out - nice find.

{% endif %}
{% if payment.provider == 'stripe' %}
<a class="btn btn-success" href="{{ url_for('.update_payment', payment_id=payment.id) }}">Update</a>
Expand Down
Loading