Skip to content

Online shop/membership #292

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.

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

Open
wants to merge 26 commits into
base: dev
Choose a base branch
from
Open

Online shop/membership #292

wants to merge 26 commits into from

Conversation

adisa39
Copy link
Collaborator

@adisa39 adisa39 commented Jul 17, 2025

Refactor @DarinHajou membership implementation to use centralized membership information for scalability and integrated Pi payment for membership.

Copy link

vercel bot commented Jul 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
map-of-pi-backend-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 21, 2025 7:04am

Copy link
Member

@DarinHajou DarinHajou Jul 18, 2025

Choose a reason for hiding this comment

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

Excellent approach! Ensure MembershipTierEnum is used throughout

Copy link
Member

Choose a reason for hiding this comment

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

Your version is clean, but I noticed updateOrRenewMembership lacks input validation to guard against malformed payloads. Do you handle this somewhere else? Consider adding:

if (
!membership_class ||
!Object.values(MembershipClassType).includes(membership_class) ||
typeof membership_duration !== 'number' ||
typeof mappi_allowance !== 'number'
) {
return res.status(400).json({ error: 'Invalid request payload' });
}

We might consider using Zod or similar down the road.

Copy link
Member

@DarinHajou DarinHajou Jul 18, 2025

Choose a reason for hiding this comment

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

Nice work. Clean logic that covers all edge cases.

  1. Looks like there’s an enum mismatch - the model uses MembershipTierEnum, but the service is still using MembershipClassType.

  2. Might be good to validate membership_class, duration, and mappi_allowance in updateOrRenewMembershipAfterPayment. If that metadata ever comes from the frontend, bad input could break stuff. A quick type check might do it.

  3. The mappi_allowance !== requiredMappi check comes after the upgrade logic. Unless that’s intentional, might be better to move it up to avoid slipping through with wrong allowance.

  4. We’re repeating the update block in every condition (existing.membership_class = ...). Might be worth wrapping that in a small helper.

  5. We might want to rename updateOrRenewMembership to something like applyMembershipChange() - it's now doing more than just update/renew; it also creates, upgrades, downgrades, and resets.

Copy link
Member

@DarinHajou DarinHajou Jul 19, 2025

Choose a reason for hiding this comment

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

Nice work.

  1. In processPaymentCompletion, we’re calling updateOrRenewMembership(currentUser, membership_class). I think that function expects the full payment object now? Might need to switch to updateOrRenewMembershipAfterPayment(currentPayment) to make sure metadata gets handled properly.

  2. We’re accessing metadata.MembershipPayment without any validation. If the frontend ever sends malformed data, it could break things or corrupt state. Probably worth adding a type check here.

  3. In buildPaymentData, looks like we assume metadata.payment_type is always present. Might be safer to add a fallback or log a warning just in case it’s missing.

  4. If payment_type is unknown, it seems to fail silently. A quick logger.warn() might help with debugging unexpected cases.

Copy link
Member

@DarinHajou DarinHajou left a comment

Choose a reason for hiding this comment

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

Left a few comments, hope it helps.

I think we should look closer at processPaymentCompletion() - becasue it's calling the old updateOrRenewMembership(currentUser, membership_class) - might need to be updateOrRenewMembershipAfterPayment(currentPayment) instead.

Not sure, but this could be why the payment failed during the demo.

@swoocn
Copy link
Member

swoocn commented Jul 19, 2025

image

It appears the Vercel serverless function is unstable because the invocation is expecting Membership model rather than membership model.

@swoocn
Copy link
Member

swoocn commented Jul 19, 2025

image It appears the Vercel serverless function is unstable because the invocation is expecting **Membership** model rather than **membership** model.
image

I went ahead and fixed the problem. ^
In the future, please make sure you run npm run build before committing the changes.

@swoocn
Copy link
Member

swoocn commented Jul 19, 2025

FYI @adisa39 - It looks like the build is currently failing due to some unit tests that weren’t updated to reflect your recent changes. I will take a look and resolve them.

Copy link
Member

@swoocn swoocn Jul 21, 2025

Choose a reason for hiding this comment

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

There are quite a number of anti-patterns in this helper file that will make it harder to maintain long-term i.e., tight coupling, duplication, and overall bloated code. Even just getting the unit tests rewritten to fix the CI/CD pipeline was a bit of a struggle because of the very procedural code structure. 😵

That said, I think it's better we take the time to clean it up now rather than let technical debt pile up. I've already started refactoring on a separate branch, so I'll plan to consolidate that work into this one and update the PR—but I'll do that after I finish reviewing the rest of the changes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants