Added the api Routes for Subscription get/post. #127
Conversation
|
Hey @AkshaTGA Thanks for opening this PR 🚀. Mentor will review your pull request soon and till then, keep contributing and stay calm. Thanks for contributing in OpenCode'25 ✨✨! |
There was a problem hiding this comment.
Pull request overview
This PR adds API routes for creating and fetching subscriptions with date-based validation logic. The implementation includes POST and GET endpoints at /api/subscriptions, with controllers that compute billing intervals and renewal dates based on user-provided start and end dates.
- Added subscription routes with GET and POST endpoints
- Implemented date utilities and billing interval calculation logic
- Modified the schema to use CommonJS module exports
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| contributors/AkshaTGA/server/src/index.js | Integrated subscription routes under /api prefix and cleaned up error formatting |
| contributors/AkshaTGA/server/src/controllers/Api.js | Added controller functions for getting and creating subscriptions with date validation and billing interval calculations |
| contributors/AkshaTGA/server/src/Routes/routes.js | Created route definitions for GET and POST /api/subscriptions endpoints |
| contributors/AkshaTGA/server/src/Models/SubscriptionSchema.js | Changed from ES6 to CommonJS module syntax and commented out userId required constraint |
| contributors/AkshaTGA/server/package.json | Added dev script for nodemon |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type: mongoose.Schema.Types.ObjectId, | ||
| ref: "User", | ||
| required: true, | ||
| // required: true, |
There was a problem hiding this comment.
The userId field's required constraint has been commented out. This allows subscriptions to be created without a userId, which could lead to orphaned subscriptions that cannot be queried or managed. This appears to be a workaround for missing authentication rather than a proper solution.
| // required: true, | |
| required: true, |
| res.status(500).json({ error: "Failed to fetch subscriptions" }); | ||
| }); | ||
| }; | ||
| //in this function, I have made is so that the next renewal date and billing interval count are calculated based on the start and end date entered byt the user. |
There was a problem hiding this comment.
Spelling error in the comment: "byt" should be "by". The comment contains a typo where "byt the user" should read "by the user".
| //in this function, I have made is so that the next renewal date and billing interval count are calculated based on the start and end date entered byt the user. | |
| //in this function, I have made is so that the next renewal date and billing interval count are calculated based on the start and end date entered by the user. |
| .then((subscriptions) => { | ||
| res.status(200).json({ subscriptions }); | ||
| }) | ||
| .catch((err) => { |
There was a problem hiding this comment.
The error handler does not log the error details. When an error occurs during subscription fetch, the error is caught but not logged, making it difficult to debug issues in production. Consider logging the error before returning the response.
| .catch((err) => { | |
| .catch((err) => { | |
| console.error("Error fetching subscriptions for user:", user._id, err); |
|
|
||
| if (!name || typeof name !== "string" || !name.trim()) { | ||
| return res.status(400).json({ error: "name is required" }); | ||
| } | ||
| if (!status) { | ||
| return res.status(400).json({ error: "status is required" }); | ||
| } | ||
| if (!billingInterval) { | ||
| return res.status(400).json({ error: "billingInterval is required" }); | ||
| } |
There was a problem hiding this comment.
The validation for status and billingInterval only checks for truthiness, not whether the values are valid according to the schema enums. For example, status must be one of "trial", "active", "paused", "cancelled", or "expired", but any truthy value would pass this check. Consider validating against the allowed enum values before attempting to create the subscription.
| if (!name || typeof name !== "string" || !name.trim()) { | |
| return res.status(400).json({ error: "name is required" }); | |
| } | |
| if (!status) { | |
| return res.status(400).json({ error: "status is required" }); | |
| } | |
| if (!billingInterval) { | |
| return res.status(400).json({ error: "billingInterval is required" }); | |
| } | |
| // derive allowed enum values from the Subscription schema, if defined | |
| const statusPath = Subscription.schema && Subscription.schema.path | |
| ? Subscription.schema.path("status") | |
| : null; | |
| const billingIntervalPath = Subscription.schema && Subscription.schema.path | |
| ? Subscription.schema.path("billingInterval") | |
| : null; | |
| const allowedStatuses = | |
| statusPath && Array.isArray(statusPath.enumValues) | |
| ? statusPath.enumValues | |
| : null; | |
| const allowedBillingIntervals = | |
| billingIntervalPath && Array.isArray(billingIntervalPath.enumValues) | |
| ? billingIntervalPath.enumValues | |
| : null; | |
| if (!name || typeof name !== "string" || !name.trim()) { | |
| return res.status(400).json({ error: "name is required" }); | |
| } | |
| if (!status) { | |
| return res.status(400).json({ error: "status is required" }); | |
| } | |
| if ( | |
| allowedStatuses && | |
| (typeof status !== "string" || !allowedStatuses.includes(status)) | |
| ) { | |
| return res.status(400).json({ error: "status is invalid" }); | |
| } | |
| if (!billingInterval) { | |
| return res.status(400).json({ error: "billingInterval is required" }); | |
| } | |
| if ( | |
| allowedBillingIntervals && | |
| (typeof billingInterval !== "string" || | |
| !allowedBillingIntervals.includes(billingInterval)) | |
| ) { | |
| return res.status(400).json({ error: "billingInterval is invalid" }); | |
| } |
| return res.status(400).json({ error: "billingInterval is required" }); | ||
| } | ||
| if (!startDate) { | ||
| return res.status(400).json({ error: "startDate is required" }); | ||
| } | ||
| if (billingInterval !== "custom" && !endDate) { | ||
| return res.status(400).json({ error: "endDate is required" }); | ||
| } | ||
|
|
||
| const parsedStartDate = toUtcDateOnly(startDate); | ||
| if (!parsedStartDate) { | ||
| return res.status(400).json({ error: "startDate must be a valid date" }); |
There was a problem hiding this comment.
The error message is inconsistent with how other field names are formatted in error messages. Throughout the code, field names in error messages are lowercase (e.g., "name is required", "status is required"), maintaining consistency would improve user experience.
| return res.status(400).json({ error: "billingInterval is required" }); | |
| } | |
| if (!startDate) { | |
| return res.status(400).json({ error: "startDate is required" }); | |
| } | |
| if (billingInterval !== "custom" && !endDate) { | |
| return res.status(400).json({ error: "endDate is required" }); | |
| } | |
| const parsedStartDate = toUtcDateOnly(startDate); | |
| if (!parsedStartDate) { | |
| return res.status(400).json({ error: "startDate must be a valid date" }); | |
| return res.status(400).json({ error: "billing interval is required" }); | |
| } | |
| if (!startDate) { | |
| return res.status(400).json({ error: "start date is required" }); | |
| } | |
| if (billingInterval !== "custom" && !endDate) { | |
| return res.status(400).json({ error: "end date is required" }); | |
| } | |
| const parsedStartDate = toUtcDateOnly(startDate); | |
| if (!parsedStartDate) { | |
| return res.status(400).json({ error: "start date must be a valid date" }); |
|
|
||
| router.get('/subscriptions', getSubscriptions ) | ||
| router.post('/subscriptions',addSubscription ) |
There was a problem hiding this comment.
The routes are missing authentication middleware. Both controllers expect req.user to be populated (lines 80 and 94 in Api.js) but there's no middleware to authenticate users and attach user information to the request. Without authentication middleware, req.user will always be undefined, causing all requests to fail with a 401 Unauthorized error.
| router.get('/subscriptions', getSubscriptions ) | |
| router.post('/subscriptions',addSubscription ) | |
| // Simple authentication middleware to ensure req.user is populated | |
| function authenticate(req, res, next) { | |
| if (!req.user) { | |
| return res.status(401).json({ error: 'Unauthorized' }); | |
| } | |
| next(); | |
| } | |
| router.get('/subscriptions', authenticate, getSubscriptions ) | |
| router.post('/subscriptions', authenticate, addSubscription ) |
| app.get("/", (req, res) => { | ||
| res.send("Server is running..."); | ||
| }); | ||
| app.use("/api",router); |
There was a problem hiding this comment.
Missing space after comma in the router middleware call. Should be '/api', router with a space after the comma for consistent formatting.
| app.use("/api",router); | |
| app.use("/api", router); |
| "description": "", | ||
| "main": "index.js", | ||
| "scripts": { | ||
| "dev":"nodemon index.js", |
There was a problem hiding this comment.
Missing space after comma in the dev script. Should be "dev": "nodemon index.js" with a space after the colon for consistent JSON formatting.
| "dev":"nodemon index.js", | |
| "dev": "nodemon index.js", |
| const computeIntervalCount = (startUtc, endUtc, billingInterval) => { | ||
| const msPerDay = 24 * 60 * 60 * 1000; | ||
| const diffMs = endUtc.getTime() - startUtc.getTime(); | ||
| if (diffMs <= 0) return null; | ||
|
|
||
| const diffDays = diffMs / msPerDay; | ||
| if (!Number.isInteger(diffDays)) return null; | ||
|
|
||
| switch (billingInterval) { | ||
| case "daily": | ||
| return diffDays + 1; | ||
| case "weekly": | ||
| return diffDays % 7 === 0 ? diffDays / 7 + 1 : null; | ||
| case "monthly": { | ||
| if (startUtc.getUTCDate() !== endUtc.getUTCDate()) return null; | ||
| const months = | ||
| (endUtc.getUTCFullYear() - startUtc.getUTCFullYear()) * 12 + | ||
| (endUtc.getUTCMonth() - startUtc.getUTCMonth()); | ||
| return months > 0 ? months + 1 : null; | ||
| } | ||
| case "yearly": { | ||
| if ( | ||
| startUtc.getUTCMonth() !== endUtc.getUTCMonth() || | ||
| startUtc.getUTCDate() !== endUtc.getUTCDate() | ||
| ) { | ||
| return null; | ||
| } | ||
| const years = endUtc.getUTCFullYear() - startUtc.getUTCFullYear(); | ||
| return years > 0 ? years + 1 : null; | ||
| } |
There was a problem hiding this comment.
The validation logic has an off-by-one error in the billing interval count calculation. The functions computeIntervalCount returns count + 1 for daily/weekly/monthly/yearly intervals (lines 54, 56, 62, 72), which includes both the start and end dates. However, this means if a subscription starts on Jan 1 and ends on Jan 2 (daily), it would count as 2 days instead of 1. This could lead to incorrect billing calculations where users are charged for an extra period.
|
I think copilot itself suggested changes in your commit , so please review and we will re - evaluate |
|
@AkshaTGA please provide screenshots as well |
4 participants
Issue: #119
Thank you for contributing to SubSentry 🚀
Please complete this template carefully to help mentors and bots review your PR efficiently.
📌 Description
Added API routes for Creation and fetching of subscriptions from the DB with Proper validation.
🧩 Issue Reference
Related Issue: #119
🛠️ What Did You Implement?
Please list the major things you worked on in this PR:
(Select all that apply)
📁 Workspace Confirmation (MANDATORY)
Since all issues are Open for All, confirm the following:
contributors/<your-github-username>/client/and/orserver/files were copied from the main directory🧪 Testing Performed
Please check what applies:
📷 Screenshots / Demo (REQUIRED for most PRs)
Please attach screenshots or recordings showing:
PRs without screenshots for significant changes may be requested for updates.
✅ Final PR Checklist
Before submitting, ensure:
.envfiles committed📎 Additional Notes for Reviewers
Add any extra context, assumptions, or known limitations here.
Thank you for contributing to SubSentry 💙
Your effort helps make this project better for everyone 🚀