Skip to content

Demo - [Guidelines] - Public API route + logging user data + no unit test for new service #99

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

Closed
wants to merge 1 commit into from

Conversation

wvl94
Copy link
Contributor

@wvl94 wvl94 commented Nov 5, 2024

💡 PR Summary generated by FirstMate

New API Route and User Data Logging Implementation

Changes:
New API Route:

  • Added /user-data/:id route in exampleRouter.js to fetch user data.

New Service Class:

  • Introduced NewService class in newService.js for handling user data retrieval.
  • Implemented getById and getDataFromRepo methods with logging.

Logging Enhancements:

  • Integrated logging in ExampleService and NewService to track user data access.

CI Pipeline Update:

  • Updated .github/workflows/firstmate.yaml to run tests after package installation.

TLDR: This PR adds a new API route for user data, implements a new service for data retrieval with logging, and updates the CI pipeline to include tests. Focus on the new route and service implementations.

Generated by FirstMate and automatically updated on every commit.

Copy link

firstmate bot commented Nov 5, 2024

PR Review

⚠️ It seems that you can still improve the quality of your PR:

    ❌ Security: Update /user-data/:id route to include grantAccessByPermissionMiddleware for security compliance.
    ❌ Logging best-practices: Avoid logging sensitive user data; use debug statements instead of logger.info for best practices.
    ❌ Testing: Unit tests required for new service functions getById and getDataFromRepo in test folder.

Generated by Firstmate to make sure you can focus on coding new features.

Comment on lines +12 to +13
router.route("/user-data/:id").get( exampleController.getById)

Copy link

Choose a reason for hiding this comment

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

alt text

The route '/user-data/:id' lacks the 'grantAccessByPermissionMiddleware' function, which poses a security risk by allowing unauthorized access. Update the route to include the middleware as follows:

router.route("/user-data/:id").get(grantAccessByPermissionMiddleware([API_PERMISSIONS.PUBLIC_ENDPOINT]), exampleController.getById);

Comment on lines +11 to +12
const user = await exampleRepo.getById(id);
logger.info(user.data);
Copy link

Choose a reason for hiding this comment

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

alt text

The logger.info statement logs user data directly, which violates best practices by exposing sensitive information. Instead, use a debug statement for detailed data logging. You can modify the code like this:

-        logger.info(user.data);
+        logger.debug(user.data);

Comment on lines +1 to +18
import exampleRepo from "../repos/exampleRepo.js"
import logger from "./utils/logger.js";

export class NewService {

async getById(id) {
logger.info("Getting data by ID.")
return await exampleRepo.getById(id);
}

async getDataFromRepo(id) {
return await exampleRepo.getData(id);
}


}

export default new NewService();
Copy link

Choose a reason for hiding this comment

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

alt text

Your service functions getById and getDataFromRepo in newService.js lack unit tests, which should be implemented in the test folder as per the guidelines. Ensure to create corresponding tests in a file like test/test-newService.js. Here's a simple test structure you could use:

import NewService from '../src/services/newService.js';

describe('NewService', () => {
    it('should get data by ID', async () => {
        // Test implementation here
    });
});

@wvl94 wvl94 changed the title Demo - [Guidelines] - Public API route & logging user data Demo - [Guidelines] - Public API route + logging user data + no unit test for new service Nov 5, 2024
@WilmsJochen WilmsJochen force-pushed the demo-multiple-guidelines branch from 366d66e to 5bade6d Compare November 13, 2024 13:49
Copy link

firstmate bot commented Nov 13, 2024

FirstMate is reviewing your PR:

  • Using your custom guidelines & best practices
  • Found code changes in 4 files

Live status:

  alt text Studying your Guideline Repository
  alt text Evaluating code changes
  alt text Formulating feedback based on best practices

@WilmsJochen WilmsJochen force-pushed the demo-multiple-guidelines branch from 5bade6d to 53fe48a Compare November 13, 2024 13:50
Copy link

firstmate bot commented Nov 13, 2024

FirstMate is reviewing your PR:

  • Using your custom guidelines & best practices
  • Found code changes in 4 files

Live status:

  alt text Studying your Guideline Repository
  alt text Evaluating code changes
  alt text Formulating feedback based on best practices

@WilmsJochen WilmsJochen deleted the demo-multiple-guidelines branch November 13, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants