Repository navigation
feat: add controller/seeds/routes and models skillup project - #34
Conversation
WalkthroughAdds a SkillUp subsystem: new Mongoose models, MongoDB init seed scripts, services and mappers, a SkillUpController with Express routes and tests, server route wiring and docker-compose DB-init volume; also adds Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Express as Server
participant Auth as Auth Middleware
participant Controller as SkillUpController
participant UserSrv as SkillUpUserService
participant StoreSrv as SkillUpStoreService
participant DB as MongoDB
Note over Client,Server: Buy product flow
Client->>Express: POST /api/v1/skillup/user/me/buy-product { productId }
Express->>Auth: authenticate request
alt authenticated
Auth->>Controller: buyProduct(req)
Controller->>UserSrv: getOrCreateUserSkillUp(user)
UserSrv->>DB: find/create UserSkillUp
DB-->>UserSrv: userSkill
Controller->>StoreSrv: buyProduct(user, productId)
StoreSrv->>DB: find Product, update userSkill (coins, itemsOwned)
DB-->>StoreSrv: updated userSkill
StoreSrv-->>Controller: updated userSkill
Controller-->>Express: 200 { data }
else unauthenticated
Auth-->>Express: 401 + OAuth hint
end
Express-->>Client: response
sequenceDiagram
actor Client
participant Express as Server
participant Auth as Auth Middleware
participant Controller as SkillUpController
participant ProgressSrv as SkillUpProgressService
participant PhaseSvc as SkillUpPhaseService
participant DB as MongoDB
Note over Client,Server: Assign played phases flow
Client->>Express: POST /api/v1/skillup/user/me/played-phases [phases]
Express->>Auth: authenticate request
alt authenticated
Auth->>Controller: assignPhasesToCurrentUser(req)
Controller->>ProgressSrv: assignPhasesToUser(user, phasesInput)
ProgressSrv->>DB: validate Phase IDs, upsert playedPhases, adjust lives
DB-->>ProgressSrv: updated userSkill
ProgressSrv-->>Controller: updated userSkill
Controller-->>Express: 200 { data }
else unauthenticated
Auth-->>Express: 401 + OAuth hint
end
Express-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vlghpp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
package.json (1)
21-21: Alignnodemonplacement with actual usageYou added
nodemonas a runtime dependency, but no script uses it (dev still runsnode --watch). Two options:
- If you intend to use nodemon for local dev, move it to
devDependenciesand update"dev"tonodemon src/server.js.- If you’ll stick with
node --watch, you can drop thenodemondependency to avoid unused bloat.docker-compose.yml (1)
15-15: Mongo init volume is correct; consider minor hardeningMounting
./src/config/mongo-initinto/docker-entrypoint-initdb.dis the right way to run the seed scripts on first DB initialization. Two optional tweaks:
- Mount as read-only (
./src/config/mongo-init:/docker-entrypoint-initdb.d:ro) to avoid accidental writes from inside the container.- Ensure all scripts inside are idempotent enough to be safe if someone reuses them manually outside the init flow.
src/models/Product.js (1)
1-18: Product schema looks solid; consider adding timestamps/indexesThe Product model is well-structured and matches the expected domain (type/rarity enums, default price, required name).
Optional hardening:
- Add
timestamps: trueto the schema options if you ever needcreatedAt/updatedAt.- Consider simple indexes (e.g., on
typeorrarity) if you’ll frequently filter/storefront by those fields.Example:
const productSchema = new Schema( { /* fields as-is */ }, { timestamps: true } );src/models/Question.js (1)
1-15: Question schema is good; you may want stricter validationThe Question model + exported
questionSchemalook consistent and reusable.If you want stronger guarantees:
- Ensure
alternativesis non-empty (e.g., custom validator on array length).- Optionally validate that
correctAlternativeis one of the entries inalternativesto avoid inconsistent data.These can be added later without breaking the current design.
src/config/mongo-init/seed-phases.js (1)
1-40: Seed script is fine; be careful withdb.phases.drop()usageFor an init script under
/docker-entrypoint-initdb.d, this works well: it ensures a cleanphasescollection and seeds consistent data.Operationally, just be aware:
- If someone runs this manually against a non-empty
sistema-aprendizagemdatabase, it will drop any existingphasescontent.- If you ever reuse this script outside the “first-start” of Mongo, consider guarding the drop (e.g., only dropping when you know you’re in a dev/demo environment).
src/routes/skillup.js (1)
1-58: SkillUp routes and auth guards look consistent; small DRY opportunityThe router cleanly exposes the SkillUp endpoints, and auth is applied where expected (public products + auth callback, protected user/game-mode/campaign routes). The arrow wrappers around controller calls are a reasonable way to keep
thisbound to the controller instance.If you ever want to reduce repetition, you could:
- Factor a small helper like
const handle = (fn) => (req, res) => fn(req, res);, or- Build the routes from a config array instead of repeating the same pattern.
Not required for merge; current code is clear as-is.
src/config/mongo-init/seed-products.js (1)
1-5: Consider usingdropIfExistspattern or documenting the destructive behavior.The
db.products.drop()call will silently fail if the collection doesn't exist (which is fine), but it will also destroy any manually added or modified product data each time the seed runs. If this is intentional for development/CI reset, consider adding a comment to clarify. If production seeding is a possibility, consider upserting instead.src/models/UserSkillUp.js (1)
25-32: Consider using Mongoose's built-intimestampsoption.The manual
createdAt/updatedAtfields with a pre-save hook can be replaced by Mongoose'stimestamps: trueoption, which handles both fields automatically and also updatesupdatedAtonfindOneAndUpdateoperations.-const userSchema = new Schema({ +const userSchema = new Schema({ name: { type: String, required: true }, // ... other fields ... - createdAt: { type: Date, default: Date.now }, - updatedAt: { type: Date, default: Date.now } -}); - -userSchema.pre('save', function(next) { - this.updatedAt = new Date(); - next(); +}, { + timestamps: true });src/models/Phase.js (2)
1-4: Minor: Group imports together at the top.The destructuring statement on line 3 separates the two import statements. Consider consolidating for consistency.
import mongoose from 'mongoose'; - -const { Schema, model } = mongoose; import { questionSchema } from './Question.js'; + +const { Schema, model } = mongoose;
9-10: Consider makinggroupandpositionrequired fields.These fields have
min/maxconstraints but are optional. The controller atgetCampaignPhaseGroups(line 115) falls back togroup || 1, implying phases without a group are unexpected. Addingrequired: trueor a default value would enforce data integrity.- group: { type: Number, min: 1, max: 5 }, - position: { type: Number, min: 1, max: 5 }, + group: { type: Number, min: 1, max: 5, required: true }, + position: { type: Number, min: 1, max: 5, required: true },src/controllers/SkillUpController.js (1)
363-428:getUserMeduplicates user creation logic.Lines 368-375 duplicate the user creation logic from
getOrCreateUserSkillUp. Consider reusing the helper function and then populating:async getUserMe(req, res) { try { - let userSkill = await UserSkillUp.findOne({ email: req.user.email }).populate('equippedItems'); - - if (!userSkill) { - userSkill = new UserSkillUp({ - name: req.user.name || req.user.email, - email: req.user.email, - coins: 0, - lifesRemaining: 5 - }); - } + let userSkill = await getOrCreateUserSkillUp(req.user); + + // Populate equippedItems if not already populated + if (userSkill.equippedItems?.length && !userSkill.populated('equippedItems')) { + await userSkill.populate('equippedItems'); + } const { modified } = applyLifeRegeneration(userSkill);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.dockerignore(1 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)src/config/mongo-init/seed-phases.js(1 hunks)src/config/mongo-init/seed-products.js(1 hunks)src/controllers/SkillUpController.js(1 hunks)src/models/Phase.js(1 hunks)src/models/Product.js(1 hunks)src/models/Question.js(1 hunks)src/models/UserSkillUp.js(1 hunks)src/routes/skillup.js(1 hunks)src/server.js(2 hunks)tests/routes/skillup.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T23:10:38.706Z
Learnt from: ederson-torresini
Repo: Extensao2/extensao-server PR: 17
File: package.json:9-17
Timestamp: 2025-11-21T23:10:38.706Z
Learning: No projeto extensao-server, os ambientes de desenvolvimento e produção suportam o uso de funções e módulos experimentais do Node.js, portanto não há necessidade de preocupações com compatibilidade cross-platform para Windows ou uso de flags experimentais como --experimental-vm-modules.
Applied to files:
src/server.js
🧬 Code graph analysis (4)
src/models/UserSkillUp.js (3)
src/models/Phase.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)src/models/Question.js (1)
mongoose(3-3)
src/models/Phase.js (1)
src/models/Question.js (3)
mongoose(3-3)questionSchema(5-11)questionSchema(5-11)
src/models/Product.js (3)
src/models/Phase.js (1)
mongoose(3-3)src/models/Question.js (1)
mongoose(3-3)src/models/UserSkillUp.js (1)
mongoose(3-3)
src/models/Question.js (3)
src/models/Phase.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)src/models/UserSkillUp.js (1)
mongoose(3-3)
🪛 ast-grep (0.40.0)
tests/routes/skillup.test.js
[warning] 13-13: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'test-secret'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(express-session-hardcoded-secret-javascript)
[warning] 29-29: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'test-secret'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(express-session-hardcoded-secret-javascript)
🔇 Additional comments (8)
.dockerignore (1)
5-5: Whitespace-only change, no action neededThe added trailing blank/whitespace line has no behavioral impact and is fine to keep or drop as you prefer.
src/server.js (1)
13-13: SkillUp routes correctly wired under existing API prefixImporting
skillUpRoutesand mounting it under/api/v1keeps routing consistent with the rest of the API. No issues here.Also applies to: 75-76
src/config/mongo-init/seed-products.js (1)
5-149: Seed data structure aligns with the Product model.The product documents correctly include all required fields (
name,type,rarity,price,url) and use appropriate data types. The categorization across headwear, gloves, footwear, and weapon types with common/rare/legendary rarities provides good variety for testing.tests/routes/skillup.test.js (3)
13-18: Hardcoded session secret is acceptable for test files.The static analysis warning about hardcoded credentials (CWE-798) is a false positive in this context. Using a fixed secret in test utilities is standard practice and does not pose a security risk since this code is never deployed to production.
Also applies to: 29-34
9-50: Well-structured test app factories.The separation of
createTestApp()(unauthenticated) andcreateAuthedTestApp()(with mock user) provides clear test isolation. The authenticated app correctly injects a mock user object matching the expected structure.
56-385: Comprehensive route coverage with consistent patterns.The test suite covers all SkillUp routes with both authentication failure (401) and success paths. The mock-and-verify pattern consistently confirms that:
- Unauthenticated requests receive 401 with OAuth URL hint
- Authenticated requests reach the controller
- Controller responses flow through correctly
src/controllers/SkillUpController.js (2)
30-76: Well-implemented life regeneration logic.The
applyLifeRegenerationfunction correctly handles:
- Maximum lives cap
- Interval-based regeneration with proper time tracking
- Remainder preservation for partial intervals
- Modified flag to avoid unnecessary saves
92-154: Efficient phase grouping implementation.The
getCampaignPhaseGroupsmethod efficiently:
- Pre-initializes all 5 groups to ensure consistent ordering
- Uses a Map for O(1) played phase lookups
- Sorts phases once at the database level
…extensao-server into feat/skill-up-project
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/SkillUpController.js(1 hunks)src/models/UserSkillUp.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/models/UserSkillUp.js (3)
src/models/Phase.js (1)
mongoose(3-3)src/models/Question.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)
src/controllers/SkillUpController.js (5)
src/models/UserSkillUp.js (1)
UserSkillUp(34-34)tests/middleware/errorHandler.test.js (1)
res(6-6)src/routes/resources.js (1)
error(22-22)src/models/Phase.js (1)
Phase(14-14)src/models/Product.js (1)
Product(16-16)
🔇 Additional comments (9)
src/models/UserSkillUp.js (1)
5-27: UserSkillUp schema structure looks consistent with controller usage.Fields (lives, coins, skill points, items, played/recommended phases, timestamps) align with how
SkillUpControllerreads and writes this model, includinglifesRemainingmax of 5 and Product/Phase refs.src/controllers/SkillUpController.js (8)
30-76: Life regeneration helper is consistent and bounded.The regeneration logic correctly caps lives at
maxLives, initializesdateLostLifewhen needed, and handles multiple elapsed intervals without overshooting the cap.
80-90: Recommendation mode gate is straightforward and defensive.Using
Array.isArrayforplayedPhasesand a simple>= 10threshold keeps this endpoint robust to missing data.
92-154: Phase grouping and played-phase overlay look coherent.You build a group map with stable IDs/names, merge in
playedPhasesvia a lookup map, and expose both structural data and per-phase progress (won,starsEarned,score,datePlayed), which should be easy for the client to consume.
251-293: Played-phases-by-email mapping and error handling look good.You validate
phasedocuments (phaseId, optionalphaseName/group/position, progress fields).
295-303: Product listing endpoint is simple and appropriate.Fetching all products with
.lean()and returning them underdatais straightforward; error handling is consistent with the rest of the controller.
336-368: Equip-item flow correctly validates ownership and avoids duplicate equips.The ownership check against
itemsOwnedand thealreadyEquippedguard using.some(id => id.toString() === product._id.toString())are appropriate for ObjectId arrays, and the 404 on missing product is handled properly.
370-435:getUserMeflow is coherent and reuses life regeneration helper well.You:
- Ensure a
UserSkillUpdoc exists (with a sensiblenamefallback).- Apply life regeneration and save only when needed.
- Populate
equippedItemsand normalize type/rarity via maps.- Return a compact payload with user meta, skill points, lives, and equipped items.
This matches the model design and should be easy for clients to work with.
437-445: Auth callback handler is simple and consistent.Redirecting to
/loginand falling back to a 500 on unexpected errors keeps this endpoint straightforward and aligned with the rest of the controller’s error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/controllers/SkillUpController.js (3)
22-36: Guard against missinguser.namewhen creatingUserSkillUp.
getOrCreateUserSkillUpstill usesname: user.nameonly. Ifuser.nameis empty while the schema requiresname,save()will fail and surface as a 500.getUserMealready falls back toreq.user.email, so these two code paths are inconsistent.Consider aligning this helper and avoiding the 500 like this:
- if (!userSkill) { - userSkill = new UserSkillUp({ - name: user.name, - email: user.email, - coins: 0, - lifesRemaining: 5 - }); - await userSkill.save(); - } + if (!userSkill) { + const name = user.name || user.email; + userSkill = new UserSkillUp({ + name, + email: user.email, + coins: 0, + lifesRemaining: 5 + }); + await userSkill.save(); + }This also keeps naming behaviour consistent with
getUserMe.
263-291: Add ObjectId validation beforePhase.findto prevent 500s on malformed IDs.
uniquePhaseIdscomes directly from client input and is passed into:const phases = await Phase.find({ _id: { $in: uniquePhaseIds } }).select('_id').lean();If any element is not a valid ObjectId string, Mongoose can throw a CastError, which becomes a 500 here. This was already flagged earlier and still appears unaddressed.
You can separate “invalid format” from “valid but non‑existent” IDs like this:
const uniquePhaseIds = Array.from(new Set(phaseIds.map(id => String(id)))); - const phases = await Phase.find({ _id: { $in: uniquePhaseIds } }).select('_id').lean(); - const validIdsSet = new Set(phases.map(phase => String(phase._id))); - - const invalidPhaseIds = uniquePhaseIds.filter(id => !validIdsSet.has(id)); - if (invalidPhaseIds.length > 0) { - return res.status(400).json({ - error: 'Some phaseIds do not exist', - invalidPhaseIds - }); - } + const invalidFormatIds = uniquePhaseIds.filter(id => !mongoose.Types.ObjectId.isValid(id)); + if (invalidFormatIds.length > 0) { + return res.status(400).json({ + error: 'Some phaseIds have invalid format', + invalidPhaseIds: invalidFormatIds + }); + } + + const phases = await Phase.find({ _id: { $in: uniquePhaseIds } }).select('_id').lean(); + const validIdsSet = new Set(phases.map(phase => String(phase._id))); + + const invalidPhaseIds = uniquePhaseIds.filter(id => !validIdsSet.has(id)); + if (invalidPhaseIds.length > 0) { + return res.status(400).json({ + error: 'Some phaseIds do not exist', + invalidPhaseIds + }); + }This keeps your existing “non‑existent ID” handling while also guarding against malformed IDs.
512-541: Handle nullproductand malformedproductIdinbuyProductto avoid 500s.
Product.findById(productId).lean()can returnnull, but the code immediately dereferencesproduct._idandproduct.price, which will throw a TypeError and surface as a 500. Malformed IDs can also trigger Mongoose CastErrors.This was called out in a previous review and is still present. A small guard fixes both cases:
const productId = req.body && req.body.productId; if (!productId) { return res.status(400).json({ error: 'productId is required' }); } - const product = await Product.findById(productId).lean(); + if (!mongoose.Types.ObjectId.isValid(productId)) { + return res.status(400).json({ error: 'Invalid productId' }); + } + + const product = await Product.findById(productId).lean(); + if (!product) { + return res.status(404).json({ error: 'Product not found' }); + } const alreadyOwns = userSkill.itemsOwned.some(id => id.toString() === product._id.toString());(Assumes you’ve added the
mongooseimport as suggested earlier.)
🧹 Nitpick comments (10)
src/models/UserSkillUp.js (1)
25-39: Consider using Mongoose's built-intimestampsoption.The manual
createdAt/updatedAtfields with a pre-save hook can be replaced by Mongoose's built-intimestampsoption, which handles this automatically and more reliably (also updates onfindOneAndUpdate, etc.).-const userSchema = new Schema({ +const userSchema = new Schema({ name: { type: String, required: true }, // ... other fields ... recommendedPhases: [{ type: Schema.Types.ObjectId, ref: 'Phase' }], - createdAt: { type: Date, default: Date.now }, - updatedAt: { type: Date, default: Date.now } -}); - -/** - * Hook de pré-save que atualiza o campo updatedAt sempre que o - * documento UserSkillUp é salvo. - * - * @this import('mongoose').Document - * @param {Function} next Função de continuação do Mongoose. - */ -userSchema.pre('save', function(next) { - this.updatedAt = new Date(); - next(); -}); +}, { timestamps: true });src/models/Question.js (1)
14-15: Consider adding validation foroptionsarray length andcorrectOptionIndexbounds.The schema allows an empty
optionsarray and acorrectOptionIndexthat could exceed the array bounds. This could lead to runtime issues when displaying questions.- options: { type: [String], required: true }, - correctOptionIndex: { type: Number, default: 0 }, + options: { + type: [String], + required: true, + validate: { + validator: (arr) => arr.length >= 2, + message: 'Questions must have at least 2 options' + } + }, + correctOptionIndex: { + type: Number, + default: 0, + min: 0, + validate: { + validator: function(val) { + return val < this.options.length; + }, + message: 'correctOptionIndex must be within options array bounds' + } + },src/models/Phase.js (1)
5-10: Consider makinggroupandpositionrequired fields.The seed data always provides
groupandpositionfor phases, suggesting they're essential for the game map structure. Without them, phases could be orphaned from the campaign navigation.const phaseSchema = new Schema({ name: { type: String, required: true }, isBossPhase: { type: Boolean, default: false }, - group: { type: Number, min: 1, max: 5 }, - position: { type: Number, min: 1, max: 5 } + group: { type: Number, min: 1, max: 5, required: true }, + position: { type: Number, min: 1, max: 5, required: true } });src/controllers/SkillUpController.js (7)
104-114: Extract the “10 played phases” threshold into a named constant.
playedCount >= 10is a domain rule that may change; moving10into a top-level constant (e.g.MIN_PHASES_FOR_RECOMMENDATION_MODE) would make it easier to tweak and reuse in tests or other endpoints.
124-186: Group aggregation looks good; consider deriving group IDs from data instead of hard‑coding 1–5.Pre-seeding
groupsMapfor groups 1–5 and then lazily creating missing groups works, but if group IDs ever change or expand, it may be simpler to derive the initial set fromallPhasesalone (or move the[1, 2, 3, 4, 5]list into shared config).
366-404: Confirm the authorization model for fetching played phases by arbitrary email.This endpoint allows callers to pass any
req.user.emailto avoid leaking other users’ progress and aiding user enumeration.
417-425: Product listing is fine; consider projecting only necessary fields for efficiency.
Product.find().lean()returns full documents; if products ever gain large descriptions/metadata, you might want to.select()only the fields the client needs (e.g. name, price, type, rarity, url) to reduce payload size.
435-502: Be aware that exposingcorrectOptionIndexenables easy client‑side cheating.For gameplay endpoints, returning
correctOptionIndexwith each question lets a modified client see all correct answers up front. If game fairness matters, consider moving answer checking server‑side and omittingcorrectOptionIndex(or only returning it after the player answers). If this route is only for internal tools, documenting that assumption would help.
551-583: Optionally normaliseequippedItemsto an array before using.some/.push.If for any reason an older
UserSkillUpdocument lacks a defaultequippedItems: [],userSkill.equippedItems.some(...)will throw. A small guard makes this more defensive:const ownsItem = userSkill.itemsOwned.some(id => id.toString() === product._id.toString()); if (!ownsItem) { return res.status(400).json({ error: 'You do not own this item' }); } - const alreadyEquipped = userSkill.equippedItems.some(id => id.toString() === product._id.toString()); + if (!Array.isArray(userSkill.equippedItems)) { + userSkill.equippedItems = []; + } + + const alreadyEquipped = userSkill.equippedItems.some(id => id.toString() === product._id.toString());If the schema guarantees a non‑null array, this is optional but still a cheap safety net.
593-653: Consider reusinggetOrCreateUserSkillUpingetUserMeto avoid duplicated creation logic.
getUserMerecreates the “newUserSkillUpwith defaults” logic that already exists ingetOrCreateUserSkillUp(with slightly different handling ofname). Consolidating user creation into the helper (possibly with an option topopulate('equippedItems')) would keep invariants likename,coins, andlifesRemainingin sync across all entry points.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/config/mongo-init/seed-phases.js(1 hunks)src/controllers/SkillUpController.js(1 hunks)src/models/Phase.js(1 hunks)src/models/Question.js(1 hunks)src/models/UserSkillUp.js(1 hunks)src/routes/skillup.js(1 hunks)tests/routes/skillup.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/skillup.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/models/Question.js (3)
src/models/Phase.js (1)
mongoose(3-3)src/models/UserSkillUp.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)
src/models/UserSkillUp.js (4)
src/models/Phase.js (1)
mongoose(3-3)src/models/Question.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)tests/middleware/errorHandler.test.js (1)
next(7-7)
src/models/Phase.js (3)
src/models/Question.js (1)
mongoose(3-3)src/models/UserSkillUp.js (1)
mongoose(3-3)src/models/Product.js (1)
mongoose(3-3)
src/controllers/SkillUpController.js (5)
src/models/UserSkillUp.js (1)
UserSkillUp(41-41)src/models/Phase.js (1)
Phase(12-12)src/config/mongo-init/seed-phases.js (3)
phaseId(206-206)questions(154-154)i(156-156)src/models/Question.js (1)
Question(24-24)src/models/Product.js (1)
Product(16-16)
🪛 ast-grep (0.40.0)
tests/routes/skillup.test.js
[warning] 13-13: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'test-secret'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(express-session-hardcoded-secret-javascript)
[warning] 29-29: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'test-secret'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(express-session-hardcoded-secret-javascript)
🔇 Additional comments (15)
src/models/UserSkillUp.js (1)
5-27: Schema structure looks good overall.The model correctly defines user state for the SkillUp feature with appropriate references to
ProductandPhasemodels. ThelifesRemainingdefault of 5 is now consistent with the controller.src/models/Question.js (1)
1-26: Question model structure is well-designed.The schema correctly references
Phase, uses appropriate enums forcategoryanddifficulty, and exports both the schema (for potential embedding) and the model.tests/routes/skillup.test.js (7)
13-18: Hard-coded session secret is acceptable for test files.The static analysis warning about
secret: 'test-secret'is a false positive in this context. Test files commonly use hard-coded secrets since they don't run in production and need deterministic behavior.Also applies to: 29-34
9-50: Well-structured test app factories.The separation of
createTestApp(unauthenticated) andcreateAuthedTestApp(authenticated via middleware injection) provides clean isolation for testing both authentication states.
56-78: Good test coverage for the products endpoint.The happy path test correctly mocks the controller and verifies both the mock invocation and response body.
81-165: Thorough auth guard testing for purchase and equip endpoints.Both
buy-productandequip-itemroutes are properly tested for 401 responses when unauthenticated and successful controller invocation when authenticated.
167-290: Comprehensive coverage for game mode and campaign endpoints.Tests properly verify authentication requirements and controller invocations for recommendation mode access, phase groups, and played phases management.
292-384: Good coverage for user and auth callback routes.The
user/me,played-phases/:email, andauth/callbackredirect are all properly tested.
386-473: Phase detail and selection mode tests are well-implemented.Both endpoints correctly test authentication guards and controller method invocations with appropriate mock responses.
src/config/mongo-init/seed-phases.js (3)
1-5: Destructive seed script drops collections unconditionally.The
drop()calls will delete all existing data. This is typical for development seeds but could be dangerous if accidentally run in production. Consider adding a safeguard or documentation.Ensure this script is only executed during initial Docker setup or development environments. Verify that production deployments do not mount or execute this initialization script.
50-89: Well-organized question pools with good variety.The HISTORY, SCIENCE, and LANGUAGES pools provide meaningful educational content with proper Portuguese localization.
114-138: Procedural math question generation is a good approach.Generating math questions dynamically ensures variety and infinite questions. The operation types (addition, subtraction, multiplication) and range constraints are appropriate for the skill level.
src/controllers/SkillUpController.js (3)
1-13: Model imports andGROUP_NAMESmapping look consistent.Importing
Producthere to supportpopulate('equippedItems')ingetUserMeis correct, and centralising campaign group names inGROUP_NAMESkeeps the rest of the controller clean.
46-92: Life regeneration logic is well-structured and side‑effect aware.The algorithm correctly handles max lives, missing timestamps, and partial refills, and the
modifiedflag lets callers avoid unnecessary writes. No issues from my side here.
668-676:authSkillupCallbackis straightforward and appropriate.Redirecting to
/loginon success and returning a 500 with logging on failure is fine for this thin callback handler.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/config/mongo-init/seed-phases.js (1)
190-209: Correct answer always at index 0 is still exploitable.This concern was raised in a previous review. The correct answer is always placed at index 0 (
correctOptionIndex: 0), which means players can exploit this by always selecting the first option.src/services/SkillUpStoreService.js (1)
1-91: Store service logic looks solid; consider ObjectId validation and optional race-safety.The overall flow for
buyProduct/equipItem(ownership checks, coin balance, and use of.some(id => id.toString() === product._id.toString())) is correct and fixes the classic ObjectId.includespitfall. Nice.Two follow-ups to consider:
- Validate ID format before
findById: Right now an invalidproductId/itemIdstring will cause Mongoose to throw a CastError, which the controller surfaces as a 500. You can mirror the earlier Phase ID review by importingmongoosehere and guarding withmongoose.Types.ObjectId.isValid(productId/itemId); if invalid, throw a dedicated error code (e.g.INVALID_PRODUCT_ID) and map it to a 400 in the controller. This keeps client errors out of the 500 bucket.- Concurrent purchase race (optional): If the same user hits
buyProducttwice in parallel for the same item, both calls can passALREADY_OWNS_PRODUCTand charge coins twice before either save completes. If this scenario is plausible, you may want to back this with a unique index on(email, productId)at the schema level or wrap the purchase in a transaction/findOneAndUpdatewith$addToSetand a balance check.src/services/SkillUpPhaseService.js (1)
18-48: Add ObjectId format handling ingetPhaseDetailDatato avoid CastError-based 500s.The method cleanly distinguishes between missing
phaseIdand a non-existent phase, but a syntactically invalidphaseId(e.g."abc") will still cause Mongoose to throw a CastError, which bubbles up as a 500 from the controller. To align with the earlier review about Phase ID validation, consider:
- Importing
mongoosehere and short-circuiting with something like:if (!mongoose.Types.ObjectId.isValid(phaseId)) { throw new Error('INVALID_PHASE_ID'); }
- Then mapping
INVALID_PHASE_IDto a 400 inSkillUpController.getPhaseDetail.This keeps client input errors out of the 500 path and makes error semantics clearer.
src/services/SkillUpUserService.js (1)
15-29: Unify new-user creation and guard against missinguser.name(reuse email as fallback).
UserSkillUpcreation is currently inconsistent:
- In
getOrCreateUserSkillUp, you usename: user.nameand immediatelysave().- In
getUserSkillWithEquippedItems, you correctly usename: user.name || user.email.If
req.user.nameis missing for a first-time user hitting any endpoint that callsgetOrCreateUserSkillUp,save()can fail on the requirednamefield and surface as a 500. You can fix the bug and de-duplicate the logic by computing a single display name and using it in both places:- async getOrCreateUserSkillUp(user) { - let userSkill = await UserSkillUp.findOne({ email: user.email }); - - if (!userSkill) { - userSkill = new UserSkillUp({ - name: user.name, - email: user.email, - coins: 0, - lifesRemaining: 5 - }); - await userSkill.save(); - } + async getOrCreateUserSkillUp(user) { + let userSkill = await UserSkillUp.findOne({ email: user.email }); + + if (!userSkill) { + const name = user.name || user.email; + userSkill = new UserSkillUp({ + name, + email: user.email, + coins: 0, + lifesRemaining: 5 + }); + await userSkill.save(); + } @@ - if (!userSkill) { - userSkill = new UserSkillUp({ - name: user.name || user.email, - email: user.email, - coins: 0, - lifesRemaining: 5 - }); - } + if (!userSkill) { + const name = user.name || user.email; + userSkill = new UserSkillUp({ + name, + email: user.email, + coins: 0, + lifesRemaining: 5 + }); + }You could also move this into a small private helper to avoid repeating the object literal.
Also applies to: 94-104
🧹 Nitpick comments (5)
src/services/SkillUpProgressService.js (1)
77-85: Consider storingphaseas ObjectId for schema consistency.Line 79 stores
phase: idwhereidis a string. If theUserSkillUp.playedPhases.phasefield is defined asObjectIdwith a ref toPhase, Mongoose will coerce it, but explicit casting is cleaner and avoids potential issues:userSkill.playedPhases.push({ - phase: id, + phase: new mongoose.Types.ObjectId(id), completed: completedProvided ? item.completed : false,This would require importing
mongooseor using thePhasemodel's ObjectId type.src/config/mongo-init/seed-phases.js (1)
116-119: Random selection may produce duplicate questions within the same phase.
getRandomFromPoolselects randomly without tracking previous selections. A phase with 10-15 questions could get the same pool question multiple times, especially since pools only have 10 questions each.Consider shuffling the pool or tracking used indices to ensure variety:
+function shuffleArray(array) { + const shuffled = [...array]; + for (let i = shuffled.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; + } + return shuffled; +}Then use shuffled pools within
generateQuestionsForPhase.src/mappers/SkillUpUserMapper.js (1)
5-32: DTO mapping is good; optionally harden defaults for unknown type/rarity.The equipped-item DTO is clean and the mapping tables are a nice touch. One minor robustness tweak you might consider: if
item.typeoritem.rarityis missing,type/rarityend upundefined. If the frontend expects strings, you could normalize to safe fallbacks, e.g.:type: TYPE_MAP[item.type] || item.type || 'UNKNOWN', rarity: RARITY_MAP[item.rarity] || item.rarity || 'COMMON'Purely optional; current behavior is acceptable if the DB is guaranteed to be consistent.
src/services/SkillUpPhaseService.js (1)
73-118: Selection-mode phase logic is sound; SUBJECT_MAP could be hoisted (optional).The selection-mode flow (subject mapping → fetch by category → dedup by statement/title → Fisher–Yates shuffle → slice to 10) looks correct and easy to follow. If you want a tiny micro-optimization/readability win, you could move
SUBJECT_MAPto a module-level constant so it isn’t reallocated per request, but that’s purely optional given how small it is.src/controllers/SkillUpController.js (1)
216-246: AlignbuyProduct/equipItemresponses withgetUserMeDTO for a consistent API.Right now
buyProductandequipItemreturn the rawuserSkilldocument fromSkillUpStoreService, whereasgetUserMeusesgetUserSkillWithEquippedItems+toUserProfileDtoto return a normalized profile DTO (with equippedItems mapped, life regeneration applied, etc.). That means clients see different shapes depending on which endpoint they hit.Consider, after a successful buy/equip:
- Either calling
SkillUpUserService.getUserSkillWithEquippedItems(req.user)and returningtoUserProfileDto(...), or- Updating
SkillUpStoreServiceto return a profile DTO directly (and adjusting the controller accordingly),so all user-skill-modifying endpoints expose the same response shape as
getUserMe. This will simplify frontend consumption and ensure life regeneration + equipped-items population are consistently applied.Also applies to: 256-286, 296-305
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/config/mongo-init/seed-phases.js(1 hunks)src/controllers/SkillUpController.js(1 hunks)src/mappers/SkillUpCampaignMapper.js(1 hunks)src/mappers/SkillUpPhaseMapper.js(1 hunks)src/mappers/SkillUpProgressMapper.js(1 hunks)src/mappers/SkillUpQuestionMapper.js(1 hunks)src/mappers/SkillUpUserMapper.js(1 hunks)src/models/Question.js(1 hunks)src/services/SkillUpPhaseService.js(1 hunks)src/services/SkillUpProgressService.js(1 hunks)src/services/SkillUpStoreService.js(1 hunks)src/services/SkillUpUserService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/models/Question.js
🧰 Additional context used
🧬 Code graph analysis (7)
src/mappers/SkillUpPhaseMapper.js (1)
src/config/mongo-init/seed-phases.js (1)
questions(167-167)
src/services/SkillUpProgressService.js (3)
src/models/Phase.js (1)
Phase(12-12)src/routes/resources.js (1)
error(22-22)src/models/UserSkillUp.js (1)
UserSkillUp(41-41)
src/services/SkillUpStoreService.js (1)
src/models/Product.js (1)
Product(16-16)
src/mappers/SkillUpCampaignMapper.js (1)
src/mappers/SkillUpProgressMapper.js (1)
playedPhasesDocs(34-34)
src/services/SkillUpUserService.js (1)
src/models/UserSkillUp.js (1)
UserSkillUp(41-41)
src/controllers/SkillUpController.js (5)
src/mappers/SkillUpCampaignMapper.js (1)
buildCampaignGroupsDto(21-71)src/mappers/SkillUpProgressMapper.js (3)
playedPhasesDocs(34-34)toPlayedPhasesAssignmentDto(12-25)toPlayedPhasesHistoryDto(33-56)src/mappers/SkillUpQuestionMapper.js (1)
toQuestionDto(10-18)src/mappers/SkillUpPhaseMapper.js (2)
toPhaseDetailDto(13-23)toSelectionModePhaseDto(32-41)src/mappers/SkillUpUserMapper.js (1)
toUserProfileDto(43-61)
src/services/SkillUpPhaseService.js (4)
src/config/mongo-init/seed-phases.js (3)
phaseId(225-225)questions(167-167)i(169-169)src/models/Phase.js (1)
Phase(12-12)src/models/Question.js (1)
Question(25-25)src/mappers/SkillUpProgressMapper.js (1)
playedPhasesDocs(34-34)
🔇 Additional comments (6)
src/mappers/SkillUpCampaignMapper.js (1)
21-70: LGTM! Clean mapper implementation.The function correctly builds the campaign groups DTO with proper Map usage for efficient lookups. The defensive handling for
playedDatafields (starsEarned, score, datePlayed) with type checks and defaults is well done.One minor consideration: the
phasesarray within each group is not sorted byposition. If ordering matters for the frontend, you may want to sort before returning:- return Array.from(groupsMap.values()); + return Array.from(groupsMap.values()).map(group => ({ + ...group, + phases: group.phases.sort((a, b) => a.position - b.position) + }));src/services/SkillUpProgressService.js (1)
100-112: LGTM!The method correctly validates the email, fetches with population, and throws appropriate errors. Clean implementation.
src/mappers/SkillUpQuestionMapper.js (1)
10-17: LGTM! Clean and defensive mapping.The mapper correctly handles the
optionsarray check and provides sensible defaults. The field renaming (statement→description,category→subject) is clear.Minor consideration: if
questioncould benull/undefinedin any calling context, a guard clause would prevent runtime errors, but this may be unnecessary if callers always provide valid documents.src/mappers/SkillUpPhaseMapper.js (1)
13-23: LGTM!The mapper correctly handles optional fields with defensive checks (
!!phase.isBossPhase,typeof phase.group === 'number'). Clean implementation.src/mappers/SkillUpProgressMapper.js (1)
1-56: Played phases mappers look correct and defensive.Both
toPlayedPhasesAssignmentDtoandtoPlayedPhasesHistoryDtohandle optional fields and non-numeric values gracefully, and the populated vs. unpopulatedphasesupport is well thought out. No changes needed from my side.src/controllers/SkillUpController.js (1)
21-51: Controller structure and error mapping look consistent and clear.The controller methods are nicely structured: each endpoint has a focused responsibility, uses the corresponding service, and maps known domain error codes (
PHASE_*,EMAIL_REQUIRED,NO_QUESTIONS_FOR_SUBJECT, etc.) to appropriate 4xx statuses, falling back to a logged 500 otherwise. The nested try/catch pattern ingetPlayedPhasesByEmailis a good way to separate service-level validation from the outer handler.Also applies to: 61-84, 94-121, 132-155, 165-173, 183-206
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/config/mongo-init/seed-phases.js (1)
190-210: Correct answer fixed at index 0 makes seeded questions easy to game.Because
optionsArrayalways has the correct answer at index 0 andcorrectOptionIndexis hardcoded to 0, any client can exploit this by always choosing the first option. This was already raised previously and still applies here.You can keep the data model unchanged and randomize only the order, e.g.:
- // REGRA DE NEGÓCIO: Resposta correta sempre no índice 0 - // Garante 4 alternativas (1 certa + 3 erradas) - const optionsArray = [ - content.c, // Index 0 (Correta) - content.w[0], // Index 1 - content.w[1], // Index 2 - content.w[2] // Index 3 - ]; + // Garante 4 alternativas (1 certa + 3 erradas) e embaralha a posição da correta + const baseOptions = [content.c, ...content.w]; + + const optionsArray = [...baseOptions]; + for (let j = optionsArray.length - 1; j > 0; j--) { + const k = Math.floor(Math.random() * (j + 1)); + [optionsArray[j], optionsArray[k]] = [optionsArray[k], optionsArray[j]]; + } + + const correctIndex = optionsArray.indexOf(content.c); questions.push({ _id: ObjectId(), phase: phaseId, category: categoryCode, topic: content.t, // Campo adicionado conforme solicitado @@ - options: optionsArray, - correctOptionIndex: 0, // Sempre 0 + options: optionsArray, + correctOptionIndex: correctIndex,If you apply this, remember to also update the header comment to no longer claim “Resposta correta sempre no índice 0”.
🧹 Nitpick comments (4)
src/mappers/SkillUpPhaseMapper.js (1)
1-41: Consider extracting a shared Phase DTO typedef/interfaceBoth functions effectively return the same Phase DTO shape; defining a shared JSDoc typedef (or TS interface in the future) for this DTO would centralize the contract and help catch drift if new fields are added later.
src/config/mongo-init/seed-phases.js (3)
14-18: Dropping collections on every run is destructive; ensure this never runs against live data.
db.phases.drop()anddb.questions.drop()followed by bulk inserts are fine for local/dev bootstrap, but they will wipe all player progress if this script is ever executed against a non-empty production database (e.g., container re-init with mounted data). Consider guarding with an environment flag or an existence/emptiness check, or having a separate non-destructive migration path for prod.Also applies to: 239-245
21-56: UseisBossPhaseinstead of phase name substring for points logic.Boss phases are already marked with
isBossPhase, but the points logic checksphaseName.includes("Chefe"). This can silently break if someone renames phases or localizes the string.Consider threading the boolean through instead:
-function generateQuestionsForPhase(phaseId, phaseName, count) { +function generateQuestionsForPhase(phaseId, phaseName, isBossPhase, count) { @@ - // Chefes valem mais pontos - points: phaseName.includes("Chefe") ? 25 : 10, + // Chefes valem mais pontos + points: isBossPhase ? 25 : 10,and at the call site:
- const questionCount = phaseData.isBossPhase ? 15 : 10; - - const newQuestions = generateQuestionsForPhase(phaseId, phaseData.name, questionCount); + const questionCount = phaseData.isBossPhase ? 15 : 10; + + const newQuestions = generateQuestionsForPhase( + phaseId, + phaseData.name, + phaseData.isBossPhase, + questionCount + );This keeps business rules tied to explicit data rather than fragile string conventions.
Also applies to: 211-216, 223-236
169-188: Category rotation and pool sampling may repeat questions; verify if that’s acceptable for gameplay.The category rotation (
CATEGORIES[i % CATEGORIES.length]) resets for each phase andgetRandomFromPoolsamples with replacement, so the same base question can appear multiple times across phases and even within a single phase. If the design goal is unique questions per phase or per game, you’ll need bookkeeping (e.g., pull‑without‑replacement within a phase or global “used” tracking). If repetition is OK for now, no change needed.Also applies to: 223-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config/mongo-init/seed-phases.js(1 hunks)src/mappers/SkillUpPhaseMapper.js(1 hunks)
🔇 Additional comments (3)
src/mappers/SkillUpPhaseMapper.js (2)
13-22: Phase detail DTO mapping is consistent and defensiveMapping looks good: id is normalized to string,
isBossPhaseis coerced to boolean, andphaseGroupfalls back tonullwhen not numeric, which should avoid consumer-side surprises.
32-40: Selection mode DTO aligns id handling with other phase DTOsUsing
String(subjectId)foridkeeps the type consistent with other phase DTOs and addresses the earlier review concern about mixed string/number ids.src/config/mongo-init/seed-phases.js (1)
126-153: Math question generator looks consistent; distractors are now non‑negative and distinct.The ranges for
a/bavoid negative results, and the wrong answers[result + 2, |result − 3|, result + 10]are always positive, unique, and different fromresult, so this addresses earlier concerns about negative/duplicate alternatives. No functional issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/services/SkillUpProgressService.js (1)
17-105: Avoid implicit historical backfill inassignPhasesToUserand consider ObjectId validation.Two points here:
Historical
datePlayedbackfill (lines 48–53).
This loop assignsnew Date()to any existingplayedPhasesentry withcompleted === truebut missingdatePlayed, even if that phase is unrelated to the current request. That:
- rewrites historical data with “now” instead of the true play date, and
- mutates extra records on every relevant call.
If this is meant as a one-off migration, it’s better as a dedicated script or a guarded, explicit migration path rather than running on every progress update.
Validate phase ID format before
Phase.find(lines 32–37).
uniquePhaseIdscome directly from the client and are passed into{ _id: { $in: uniquePhaseIds } }. With malformed IDs, Mongoose can throw aCastError, which your controller will surface as a 500 instead of a 4xx client error. To harden this:-import Phase from '../models/Phase.js';
+import mongoose from 'mongoose';
+import Phase from '../models/Phase.js';
@@
- const uniquePhaseIds = Array.from(new Set(phaseIds.map(id => String(id))));
- const allIds = phaseIds.map(id => String(id));
- const syntacticallyValidIds = allIds.filter(id => mongoose.Types.ObjectId.isValid(id));
- const uniquePhaseIds = Array.from(new Set(syntacticallyValidIds));
- const invalidFormatIds = allIds.filter(id => !mongoose.Types.ObjectId.isValid(id));
- if (invalidFormatIds.length > 0) {
const error = new Error('INVALID_PHASE_IDS');// @ts-ignoreerror.invalidPhaseIds = invalidFormatIds;throw error;- }
Then keep your existing “does not exist in DB” check for valid-but-missing IDs. </blockquote></details> <details> <summary>src/services/SkillUpUserService.js (1)</summary><blockquote> `15-29`: **Guard against missing `user.name` when creating `UserSkillUp`.** Here you create a new `UserSkillUp` with `name: user.name` but no fallback, while `getUserSkillWithEquippedItems` already uses `user.name || user.email`. If `req.user.name` is missing/empty, this `save()` can fail due to schema validation and surface as a 500 on the first interaction for that user. Align this with the other method: ```diff - if (!userSkill) { - userSkill = new UserSkillUp({ - name: user.name, - email: user.email, - coins: 0, - lifesRemaining: 5 - }); - await userSkill.save(); - } + if (!userSkill) { + userSkill = new UserSkillUp({ + name: user.name || user.email, + email: user.email, + coins: 0, + lifesRemaining: 5 + }); + await userSkill.save(); + }
🧹 Nitpick comments (4)
src/routes/skillup.js (2)
68-72: Re-check access control for/skillup/user/played-phases/:email.This endpoint is protected by
requireAuthbut allows any authenticated user to query any email’s play history. If that history is not meant to be visible cross-user, you likely want to either:
- restrict to the current user (
req.user.email) only, or- add a role/permission check (e.g., admin/teacher) before calling
getPlayedPhasesByEmail.
80-88: Confirm whether the OAuth callback should also invokepassport.authenticate.The start route correctly uses
passport.authenticate('google', { scope: [...] }), but the callback route just callsSkillUpController.authSkillupCallbackwithout runningpassport.authenticate. If/skillup/auth/callbackis intended to be Google’s actual callback URL, Passport will never complete the login flow here.Typical pattern:
router.get( '/skillup/auth/callback', passport.authenticate('google', { failureRedirect: '/login' }), (req, res) => SkillUpController.authSkillupCallback(req, res) );If the real OAuth callback is handled elsewhere and this is only a post-login redirect, you can ignore this, but please confirm.
src/controllers/SkillUpController.js (2)
21-331: Controller-service wiring and error mapping look solid; only minor polish possible.The controller methods consistently:
- delegate business logic to the SkillUp services,
- translate well-defined
error.messagecodes into appropriate 400/404 responses, and- reserve 500 for unexpected failures with logging.
This keeps HTTP concerns cleanly separated from domain logic and mappers.
If you later want to reduce repetition, you could:
- extract a small helper to turn
{ message, data }from services into{ status, body }, and/or- centralize the repeated
console.error(..., error); return res.status(500).json({ error: 'Internal server error' });pattern.No changes required for correctness.
342-350:authSkillupCallbackcan be simplified if no async work is expected.Given this handler just calls
res.redirect('/login'), the try/catch is effectively redundant—Express will already handle unexpected errors. You can simplify to:authSkillupCallback(req, res) { return res.redirect('/login'); }(If you later add async work or rely on Passport here, you can reintroduce error handling.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/controllers/SkillUpController.js(1 hunks)src/mappers/SkillUpProgressMapper.js(1 hunks)src/routes/skillup.js(1 hunks)src/server.js(2 hunks)src/services/SkillUpProgressService.js(1 hunks)src/services/SkillUpUserService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mappers/SkillUpProgressMapper.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/services/SkillUpUserService.js (1)
src/models/UserSkillUp.js (1)
UserSkillUp(41-41)
src/services/SkillUpProgressService.js (4)
src/mappers/SkillUpProgressMapper.js (1)
phases(35-39)src/models/Phase.js (1)
Phase(12-12)src/routes/resources.js (1)
error(22-22)src/models/UserSkillUp.js (1)
UserSkillUp(41-41)
src/controllers/SkillUpController.js (5)
src/mappers/SkillUpCampaignMapper.js (1)
buildCampaignGroupsDto(21-71)src/mappers/SkillUpProgressMapper.js (3)
playedPhasesDocs(51-51)toSimplePlayedPhasesDto(34-42)toPlayedPhasesHistoryDto(50-73)src/mappers/SkillUpQuestionMapper.js (1)
toQuestionDto(10-18)src/mappers/SkillUpPhaseMapper.js (2)
toPhaseDetailDto(13-23)toSelectionModePhaseDto(32-41)src/mappers/SkillUpUserMapper.js (1)
toUserProfileDto(43-61)
🔇 Additional comments (1)
src/server.js (1)
13-13: SkillUp router mounting looks correct and non-disruptive.Importing
skillUpRoutesand mounting it under the existing/api/v1prefix keeps the API structure consistent with other modules and doesn’t interfere with health/error handlers.Also applies to: 61-62
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/SkillUpProgressService.js (1)
48-53: Side effect: backfillingdatePlayedon every call may overwrite historical accuracy.This loop sets
datePlayed = new Date()for any existing entry wherecompletedis true butdatePlayedis missing. This runs on everyassignPhasesToUsercall, which means old entries get today's date instead of their actual play date.Consider running this as a one-time migration script instead, or remove the backfill logic entirely if the original dates are lost anyway.
🧹 Nitpick comments (2)
src/controllers/SkillUpController.js (2)
8-8: Remove unused importtoPlayedPhasesAssignmentDto.This import is not used anywhere in the controller.
-import { toPlayedPhasesAssignmentDto, toPlayedPhasesHistoryDto, toSimplePlayedPhasesDto } from '../mappers/SkillUpProgressMapper.js'; +import { toPlayedPhasesHistoryDto, toSimplePlayedPhasesDto } from '../mappers/SkillUpProgressMapper.js';
343-351: Remove unnecessary try-catch around synchronous redirect.
res.redirect()is synchronous and doesn't throw under normal conditions. If an error did occur after the redirect started, the catch block'sres.status(500).json()would fail with "headers already sent", masking the original error.authSkillupCallback(req, res) { - try{ - return res.redirect('/login'); - } - catch(error){ - console.error('Error on /skillup/auth/callback:', error); - return res.status(500).json({ error: 'Internal server error' }); - } + return res.redirect('/login'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/SkillUpController.js(1 hunks)src/mappers/SkillUpUserMapper.js(1 hunks)src/models/UserSkillUp.js(1 hunks)src/services/SkillUpProgressService.js(1 hunks)src/services/SkillUpUserService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mappers/SkillUpUserMapper.js
- src/services/SkillUpUserService.js
- src/models/UserSkillUp.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/controllers/SkillUpController.js (5)
src/mappers/SkillUpCampaignMapper.js (1)
buildCampaignGroupsDto(21-71)src/mappers/SkillUpProgressMapper.js (3)
playedPhasesDocs(51-51)toSimplePlayedPhasesDto(34-42)toPlayedPhasesHistoryDto(50-73)src/mappers/SkillUpQuestionMapper.js (1)
toQuestionDto(10-18)src/mappers/SkillUpPhaseMapper.js (2)
toPhaseDetailDto(13-23)toSelectionModePhaseDto(32-41)src/mappers/SkillUpUserMapper.js (1)
toUserProfileDto(43-62)
src/services/SkillUpProgressService.js (4)
src/mappers/SkillUpProgressMapper.js (1)
phases(35-39)src/models/Phase.js (1)
Phase(12-12)src/routes/resources.js (1)
error(22-22)src/models/UserSkillUp.js (1)
UserSkillUp(42-42)
🔇 Additional comments (8)
src/services/SkillUpProgressService.js (3)
1-9: LGTM!Imports and class structure are well-organized. The JSDoc documentation in Portuguese is consistent with the codebase style.
57-105: LGTM!The phase processing logic correctly handles both updates and creates for playedPhases entries. The life deduction mechanism when
completed === falseappropriately implements the game's penalty for failed attempts.
114-126: LGTM!Clean implementation with proper validation and error handling. The population of
playedPhases.phaseenables the mapper to build the history DTO with phase metadata.src/controllers/SkillUpController.js (5)
21-51: LGTM!Both
canAccessRecommendationModeandgetCampaignPhaseGroupscleanly delegate to their respective services and handle errors appropriately.
94-122: LGTM!Error handling is comprehensive with appropriate HTTP status codes. The inclusion of
invalidPhaseIdsin the 400 response provides useful debugging information to API consumers.
165-246: LGTM!
getProducts,getSelectionModePhase, andbuyProductfollow consistent patterns with comprehensive error handling. The nested try-catch inbuyProductcleanly separates service-level errors from unexpected exceptions.
256-333: LGTM!
equipItem,getUserMe, andaddLifeToCurrentUserare well-implemented with appropriate error handling and DTO mapping.
132-156: Verify intentional API design: endpoint allows querying any user's played phasesThe endpoint requires authentication (
requireAuthmiddleware) but does not restrict access to the current user's data. Line 134 accepts email fromreq.params.email, enabling any authenticated user to request another user's played phases via/skillup/user/played-phases/:email. If this is intentional (e.g., a public admin endpoint), document it. If not, restrict toreq.user.emailto match the pattern of/skillup/user/me/played-phases.
Modelo de Pull Request
Descrição
Adicionado os controllers, seeds, rotas e modelos do projeto skillup
Tipos de Mudanças
Como o teste foi realizado?
Checklist:
Summary by CodeRabbit
New Features
Backend
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.