Skip to content

Demo: add log of user data and public API route. #110

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.

Sign up for GitHub

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
Closed

Demo: add log of user data and public API route. #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions .github/workflows/firstmate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,6 @@ jobs:
run: pnpm lint
- name: Install packages
run: pnpm test
docker:
runs-on: ubuntu-latest
name: Validate docker
steps:
- uses: actions/checkout@v3
- name: Setup Docker buildx
uses: docker/[email protected]
with:
driver: docker
- name: docker build
run: docker build . -t "test-docker"
helm:
runs-on: ubuntu-latest
name: Validate helm
Expand Down
2 changes: 2 additions & 0 deletions src/routes/exampleRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ const router = express.Router();
router.route("/:id")
.get(grantAccessByPermissionMiddleware([API_PERMISSIONS.PUBLIC_ENDPOINT]), exampleController.getById)

router.route("/user-data/:id").get( exampleController.getById)

export default router;
Comment on lines +12 to 14
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 does not use the grantAccessByPermissionMiddleware, which is required for security purposes. You should modify the route to include this middleware as follows:

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

This ensures that all routes check permissions properly.

(Based on guideline 'Check permissions with middleware')

Comment on lines +12 to 14
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 does not utilize the grantAccessByPermissionMiddleware. You should add this middleware to ensure proper permission checks, like this:

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

(Based on guideline 'Routes should check permissions')

6 changes: 6 additions & 0 deletions src/services/exampleService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import exampleRepo from "../repos/exampleRepo.js"
import logger from "../utils/logger.js";

export class ExampleService {

async getById(id) {
return await exampleRepo.getById(id)
}

async getUserData(id) {
const user = await exampleRepo.getById(id);
logger.info(user.data);
}
Comment on lines +11 to +13
Copy link

Choose a reason for hiding this comment

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

alt text

Logging business data at info level is not recommended as per the guidelines. Consider using a different log level, such as debug, for logging business data. Change logger.info(user.data); to logger.debug(user.data); in the getUserData method.

(Based on guideline 'No business data on info level')

Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

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

alt text

Logging user data at info level is against the guideline. Consider using debug level for logging business data. Replace logger.info(user.data); with logger.debug(user.data);.

(Based on guideline 'Never log business data on info level')



}

Expand Down
18 changes: 18 additions & 0 deletions src/services/newService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,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);
}
Comment on lines +4 to +13
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 new service functions getById and getDataFromRepo lack corresponding unit tests, which are essential for ensuring functionality and reliability. You should implement unit tests in the test folder to cover these functions, similar to the existing unit tests in test/test-user.js. Here's a basic structure for your tests:

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

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

    it('should get data from repo', async () => {
        // Your test logic here
    });
});

(Based on guideline 'Unit tests for service functions')



}

export default new NewService();
Comment on lines +1 to +18
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 newService.js file lacks corresponding unit tests in the test folder. Implement unit tests to cover the functionality of the NewService class, similar to how it's done in test/test-user.py. Here's a basic structure for your test file:

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

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

(Based on guideline 'Services should have unit tests')

Loading