Skip to content

Top Up Products#12

Open
BenBals wants to merge 6 commits intomainfrom
topup-products
Open

Top Up Products#12
BenBals wants to merge 6 commits intomainfrom
topup-products

Conversation

@BenBals
Copy link
Copy Markdown
Collaborator

@BenBals BenBals commented Mar 15, 2024

By marking a pretix products as "top up", buying them will charge a users wallet by their value. This PR also introduces a wallet membership type, automatically creates and assigns it to wallet users. You can use this to ensure users buying top up products are forced to sign in.

It also introduces a redirect url route using which you can funnel users through the wallet login and them redirect them to an arbitrary target. This is useful for linking users to top-up shops since it ensures the correct data (user, wallet, membership type, membership) are created before the user enters the shop.

@BenBals
Copy link
Copy Markdown
Collaborator Author

BenBals commented Mar 15, 2024

What's still left todo: How to integrate this into the front end?

  • @dasGoogle What would be a good visual design?
    • idea: if you're in the negative, the terminal shows you a big QR code to pay after checkout
    • similarly, we could make the button in the front end more prominent if people are in the negative
    • both of these ideas could be adapted to the old flow if we decide against this pr
  • @jeriox What is a good data model for keeping track of the "top-up event"? The frontend somehow needs to know where to send people, but payment provider settings are on the event level. Is there a way to have organiser-level settings? Do we need to rely on magical event slugs? That would be ugly.

def get_or_create_wallet_membership_type(organizer):
membership_type = organizer.membership_types.filter(name=membership_type_name_for_organizer(organizer)).first()

if membership_type is not None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.filter(...).exists() is the prettier sytnax

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If I read the docs correctly, that doesn't give me the object (which I want to return, because I use it in lines 49 and 53). But in line 49, I can use exists. I've changed that.

try:
gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gcpk)
except GiftCard.DoesNotExist:
raise PaymentException(_("This gift card does not support this currency."))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

exception text is wrong (not your fault)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What should it say?

payment.info_data['transaction_id'] = trans.pk
payment.confirm(send_mail=not is_early_special_case, generate_invoice=not is_early_special_case)
try:
gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gcpk)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.get() evaluates the queryset, which activates the lock. if you want to keep it locked, the evaluation needs to happen inside the same transaction as the other operations

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

is there a reason to keep the lock? Shouldn't the lock around the write part be enough? Ore are you worried about the gift card changing owners?


@receiver(order_paid, dispatch_uid="payment_wallet_order_paid")
def wallet_order_paid(sender, order, **kwargs):
CustomerWallet.create_if_non_existent(sender.organizer, order.customer)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

all of this should only happen if there is a topup product

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

return Response(WalletSerializer(self.wallet).data, status=HTTP_201_CREATED)


class WalletRequiredRedirectView(CustomerRequiredMixin, WalletRequiredMixin, View):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'll keep this until we've verified the checkout step actually works. If that's the case, we can remove it.

'for the top-up product?')

def is_completed(self, request, warn=False):
if self.request.customer.wallet:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC this raises if not existent

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this should never happen (see get). This I intended this to raise as this ever happening is a bug. Then, some of our central assumptions must be pretty wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants