-
Notifications
You must be signed in to change notification settings - Fork 3
PM-3329 member data #41
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
Conversation
… returns the skill data + activity for the skill
…into PM-3329_member-data
| description: |- | ||
| Get a specific member skill by skill ID. | ||
| Authorization: |
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.
[💡 readability]
The description for the endpoint mentions 'secure and communication fields' but does not specify what these fields are. Consider clarifying which fields are considered secure and communication fields to avoid ambiguity.
| - Public: Without a token responses omit secure and communication fields. | ||
| - JWT roles: The profile owner or `administrator`/`admin` roles may view secure fields. | ||
| - M2M scopes: `read:user_profiles` or `all:user_profiles`. | ||
| security: |
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.
[security]
The security definition uses 'bearer' which is typically associated with JWT tokens. Ensure that this aligns with the intended authentication mechanism and that the necessary scopes are properly documented and enforced.
| '200': | ||
| description: OK | ||
| schema: | ||
| $ref: '#/definitions/MemberSkillsWithActivity' |
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.
[❗❗ correctness]
The response schema references 'MemberSkillsWithActivity'. Ensure that this definition is correctly implemented and includes all necessary fields to avoid runtime errors.
| example: 'Skill verified by proof of capability' | ||
|
|
||
| MemberSkillsWithActivity: | ||
| allOf: |
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.
[❗❗ correctness]
The 'MemberSkillsWithActivity' definition extends 'MemberSkills'. Verify that all required fields in 'MemberSkills' are included and correctly mapped in 'MemberSkillsWithActivity' to prevent data inconsistencies.
| - type: object | ||
| properties: | ||
| activity: | ||
| type: object |
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.
[design]
The 'activity' object includes nested objects for 'challenge', 'certification', and 'course'. Consider adding validation rules to ensure that these objects are populated with valid data to maintain data integrity.
src/common/prisma.js
Outdated
| const getChallengesClient = () => { | ||
| if (!challengesClient) { | ||
| if (!challengesDbUrl) { | ||
| throw new Error('CHALLENGES_DB_URL must be set for skills Prisma client') |
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.
[correctness]
The error message here states 'CHALLENGES_DB_URL must be set for skills Prisma client', which seems incorrect. It should refer to the challenges Prisma client instead.
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.
Samne here
src/common/prisma.js
Outdated
| const getAcademyClient = () => { | ||
| if (!academyClient) { | ||
| if (!academyDbUrl) { | ||
| throw new Error('ACADEMY_DB_URL must be set for skills Prisma client') |
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.
[correctness]
The error message here states 'ACADEMY_DB_URL must be set for skills Prisma client', which seems incorrect. It should refer to the academy Prisma client instead.
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.
@vas3a please update typo. Should be academy client.
| if (first.skill && first.skill.skillEvents) { | ||
| const events = _.orderBy(first.skill.skillEvents || [], 'createdAt', 'desc') | ||
| const grouped = _.groupBy(events, 'sourceType.name') | ||
| ret.lastUsedDate = events[0].createdAt |
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.
[❗❗ correctness]
Accessing events[0] without checking if events is non-empty could lead to a runtime error. Consider adding a check to ensure events has at least one element before accessing it.
| * @param {Object} res the response | ||
| */ | ||
| async function getMemberSkill (req, res) { | ||
| const result = await service.getMemberSkill(req.authUser, req.params.handle, req.params.skillid) |
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.
[❗❗ correctness]
Consider adding error handling for the getMemberSkill function. If service.getMemberSkill throws an error, it will not be caught, potentially leading to unhandled promise rejections and server crashes.
src/routes.js
Outdated
| controller: 'MemberController', | ||
| method: 'getMemberSkill', | ||
| auth: 'jwt', | ||
| allowNoToken: true, |
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.
[❗❗ security]
The allowNoToken: true setting allows access without authentication. Ensure this is intentional and aligns with security requirements, as it could expose sensitive data.
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.
@vas3a please remove public access. Any user role could fetch but annonymous should be excluded.
| if (certificationIds.length > 0) { | ||
| fetchPromises.push( | ||
| academyPrisma.CertificationEnrollments.findMany({ | ||
| where: { completionEventId: { in: certificationIds.slice(0, 3) } }, |
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.
[💡 maintainability]
Consider limiting the number of certification IDs fetched to a configurable constant instead of hardcoding 3. This will make the code more flexible and easier to maintain.
kkartunov
left a comment
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.
Few minors.
src/routes.js
Outdated
| controller: 'MemberController', | ||
| method: 'getMemberSkill', | ||
| auth: 'jwt', | ||
| allowNoToken: true, |
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.
@vas3a please remove public access. Any user role could fetch but annonymous should be excluded.
src/common/prisma.js
Outdated
| const getAcademyClient = () => { | ||
| if (!academyClient) { | ||
| if (!academyDbUrl) { | ||
| throw new Error('ACADEMY_DB_URL must be set for skills Prisma client') |
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.
@vas3a please update typo. Should be academy client.
src/common/prisma.js
Outdated
| const getChallengesClient = () => { | ||
| if (!challengesClient) { | ||
| if (!challengesDbUrl) { | ||
| throw new Error('CHALLENGES_DB_URL must be set for skills Prisma client') |
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.
Samne here
kkartunov
left a comment
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.
Just a minor tweak.
| const dbSkill = await skillsPrisma.userSkill.findFirst({ | ||
| where: { | ||
| userId: helper.bigIntToNumber(member.userId), | ||
| skillId: skillId |
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.
Let's ensure skillsId is UUID before passing this to prisma query for security concerns and SQL injections.
| getMemberSkill.schema = { | ||
| currentUser: Joi.any(), | ||
| handle: Joi.string().required(), | ||
| skillId: Joi.string().uuid().required() |
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.
[correctness]
Changing skillId to be validated as a UUID is a good improvement for ensuring data integrity. However, ensure that all parts of the system that interact with this endpoint are updated to provide skillId in UUID format to prevent any unexpected errors.
…into PM-3329_member-data
| const skillsDbUrl = process.env.SKILLS_DB_URL | ||
| const challengesDbUrl = process.env.CHALLENGES_DB_URL | ||
| const academyDbUrl = process.env.ACADEMY_DB_URL | ||
| const resourcesDbUrl = config.RESOURCES_DB_URL |
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.
[maintainability]
Consider using process.env.RESOURCES_DB_URL instead of config.RESOURCES_DB_URL for consistency with other database URL configurations. This will also ensure that the URL can be easily configured via environment variables, which is a common practice for managing sensitive information like database connection strings.
Add new endpoint that returns "Activity" for a skill: where, & when it was earned.
Related PRs: