Online shop/membership #292
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import { Request, Response } from "express"; | ||
| import * as membershipService from "../services/membership.service"; | ||
| import logger from "../config/loggingConfig"; | ||
| import { IUser } from "../types" | ||
|
|
||
| export const fetchMembershipList = async (req: Request, res: Response) => { | ||
|
There was a problem hiding this comment. @DarinHajou/ @adisa39 - is it necessary to build an endpoint to fetch what’s essentially static, public-facing data? From what I understand, the PM mentioned that the membership tiers are mostly set and unlikely to change. If that’s the case, wouldn’t it be more efficient and straightforward to hardcode these tiers on the FE instead of fetching them from the BE on every page load? 🤔 There was a problem hiding this comment. I agree. This is divergin a bit from the original implementation. No need for an endpoint here as the memebership tiers will unlikely change much. |
||
| try { | ||
| const membershipList = await membershipService.buildMembershipList(); | ||
|
|
||
| if (!membershipList) { | ||
swoocn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| logger.warn(`No membership list found.`); | ||
| return res.status(404).json({ message: "Membership list not found" }); | ||
| } | ||
|
|
||
| logger.info(`Fetched membership list`); | ||
| return res.status(200).json(membershipList); | ||
|
|
||
| } catch (error) { | ||
| logger.error(`Error getting membership list: `, error); | ||
| return res.status(500).json({ message: 'An error occurred while getting single membership list; please try again later' }); | ||
| } | ||
| }; | ||
|
|
||
| export const fetchUserMembership = async (req: Request, res: Response) => { | ||
| const authUser = req.currentUser as IUser; | ||
|
|
||
| try { | ||
| const currentMembership = await membershipService.getUserMembership(authUser); | ||
|
|
||
| if (!currentMembership) { | ||
| logger.warn(`Membership with ID ${authUser.pi_uid} not found.`); | ||
| return res.status(404).json({ message: "Membership not found" }); | ||
| } | ||
|
|
||
| logger.info(`Fetched membership with ID ${authUser.pi_uid}`); | ||
| return res.status(200).json(currentMembership); | ||
| } catch (error) { | ||
| logger.error(`Error getting membership ID ${authUser.pi_uid}:`, error); | ||
| return res.status(500).json({ message: 'An error occurred while getting single membership; please try again later' }); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| // Controller to fetch a single membership by its ID | ||
| export const getSingleMembership = async (req: Request, res: Response) => { | ||
| const { membership_id } = req.params; | ||
|
|
||
| try { | ||
| const membership = await membershipService.getSingleMembershipById(membership_id); | ||
|
|
||
| if (!membership) { | ||
| logger.warn(`Membership with ID ${membership_id} not found.`); | ||
| return res.status(404).json({ message: "Membership not found" }); | ||
| } | ||
|
|
||
| logger.info(`Fetched membership with ID ${membership_id}`); | ||
| return res.status(200).json(membership); | ||
| } catch (error) { | ||
| logger.error(`Error getting membership ID ${membership_id}:`, error); | ||
| return res.status(500).json({ message: 'An error occurred while getting single membership; please try again later' }); | ||
| } | ||
| }; | ||
|
|
||
| export const updateOrRenewMembership = async (req: Request, res: Response) => { | ||
| try { | ||
| const { membership_class } = req.body; | ||
| const authUser = req.currentUser | ||
|
|
||
| if (!authUser) return res.status(401).json({ error: "Unauthorized - user not authenticated "}); | ||
|
|
||
| const updated = await membershipService.updateOrRenewMembership(authUser.pi_uid, membership_class); | ||
|
|
||
| return res.status(200).json(updated); | ||
| } catch (error: any) { | ||
| logger.error("Failed to update or renew membership:", error); | ||
| return res.status(400).json({ error: error.message }); | ||
swoocn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,20 +5,22 @@ import * as userService from "../services/user.service"; | |
| import { IUser } from "../types"; | ||
|
|
||
| import logger from '../config/loggingConfig'; | ||
| import { getUserMembership } from "../services/membership.service"; | ||
|
|
||
| export const authenticateUser = async (req: Request, res: Response) => { | ||
|
There was a problem hiding this comment. @DarinHajou / @adisa39 - Good call including membership_class in the authentication response — that makes sense given the app relies on this info early in the session. If a user upgrades or downgrades their membership during the session, are we currently refreshing or updating the cached membership_class on the frontend? Just want to make sure we avoid cases where stale membership data might impact downstream features or expected UI visibility. There was a problem hiding this comment. Valid concern. I believe the best approach to accommodate that is to directly update currentUser.membership_class in frontend state after the payment response |
||
| const auth = req.body; | ||
|
|
||
| try { | ||
| const result = await userService.authenticate(auth.user); | ||
| const token = jwtHelper.generateUserToken(result.user); | ||
| const expiresDate = new Date(Date.now() + 1 * 24 * 60 * 60 * 1000); // 1 day | ||
|
|
||
| logger.info(`User authenticated: ${result.user.pi_uid}`); | ||
|
|
||
| return res.cookie("token", token, {httpOnly: true, expires: expiresDate, secure: true, priority: "high", sameSite: "lax"}).status(200).json({ | ||
| user: result.user, | ||
| token, | ||
| membership_class: result.membership_class | ||
| }); | ||
| } catch (error) { | ||
| logger.error('Failed to authenticate user:', error); | ||
|
|
@@ -28,9 +30,10 @@ export const authenticateUser = async (req: Request, res: Response) => { | |
|
|
||
| export const autoLoginUser = async(req: Request, res: Response) => { | ||
| try { | ||
| const currentUser = req.currentUser as IUser; | ||
| const membership = await getUserMembership(currentUser); | ||
|
|
||
| logger.info(`Auto-login successful for user: ${currentUser?.pi_uid || "NULL"}`); | ||
| res.status(200).json({user: currentUser, membership_class: membership.membership_class}); | ||
| } catch (error) { | ||
| logger.error(`Failed to auto-login user for userID ${ req.currentUser?.pi_uid }:`, error); | ||
| return res.status(500).json({ message: 'An error occurred while auto-logging the user; please try again later' }); | ||
|
|
||
|
There was a problem hiding this comment. Nice work.
There was a problem hiding this comment. 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. There was a problem hiding this comment. I'll be working on refactoring this helper file. 😵 |
swoocn marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { NextFunction, Request, Response } from "express"; | ||
|
|
||
| import Membership from "../models/Membership"; | ||
| import { IMembership } from "../types"; | ||
|
|
||
| import logger from '../config/loggingConfig'; | ||
|
|
||
| declare module 'express-serve-static-core' { | ||
| interface Request { | ||
| currentMembership: IMembership; | ||
| } | ||
| } | ||
|
|
||
| export const isMembershipFound = async ( | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction | ||
| ) => { | ||
| const membership_id = req.currentUser?.pi_uid; | ||
|
|
||
| try { | ||
| logger.info(`Checking if membership exists for user ID: ${membership_id}`); | ||
| const currentMembership: IMembership | null = await Membership.findOne({pi_uid: membership_id}); | ||
|
|
||
| if (currentMembership) { | ||
| req.currentMembership = currentMembership; | ||
| logger.info(`Membership found: ${currentMembership._id}`); | ||
| return next(); | ||
| } else { | ||
| logger.warn(`Membership not found for user ID: ${membership_id}`); | ||
| return res.status(404).json({message: "Membership not found"}); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to identify membership:', error); | ||
| res.status(500).json({ message: 'Failed to identify | membership not found; please try again later'}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! Feel free to implement and add a unit test for it @DarinHajou.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, let's plan to enhance all our controllers with stronger server-side validation during the upcoming code cleanup and technical debt task.