Skip to content
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

Feat/expense split app #146

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

arnb-smnta
Copy link
Contributor

Hey @jwala-anirudh @wajeshubham This is draft PR for review of models routes and aggregation pipelines and helper functions This is mostly a work in progress most of the controllers are not tested yet as reviewed by anirudh in his comment #127 I am uploading it for initial review I will be using socket io for emitting expense group events after testing of all the routes thoroughly as socket io and configs are already present in the project I will integrating it at the very end

@wajeshubham
Copy link
Collaborator

wajeshubham commented Apr 28, 2024

@arnb-smnta kindly go through the naming convention we are following for FreeAPI we are not following camel casing in file names and routes. Also, let us know once the PR is ready for review. Thank you!

@arnb-smnta
Copy link
Contributor Author

@wajeshubham @jwala-anirudh Its ready for review

@arnb-smnta
Copy link
Contributor Author

Run In Postman This the link to postman collection for your convienience

@jwala-anirudh
Copy link
Collaborator

jwala-anirudh commented May 1, 2024

Why is yarn.lock deleted? Please revert it back to its original state

Copy link
Collaborator

@jwala-anirudh jwala-anirudh left a comment

Choose a reason for hiding this comment

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

Cannot delete yarn.lock file from root

@arnb-smnta
Copy link
Contributor Author

@jwala-anirudh please see my new pr and see if it isok

@wajeshubham
Copy link
Collaborator

@arnb-smnta kindly go through the naming convention we are following for FreeAPI we are not following camel casing in file names and routes. Also, let us know once the PR is ready for review. Thank you!

Please address this

@jwala-anirudh
Copy link
Collaborator

@jwala-anirudh please see my new pr and see if it isok

I've checked and still cannot see the recovery of yarn.lock file here

Screenshot

@arnb-smnta
Copy link
Contributor Author

@wajeshubham I have addressed the naming problem i think and also the yarn.lock problem i figure it out I think @jwala-anirudh its added can u just let me know if both the problems are addressed properly ?

@arnb-smnta
Copy link
Contributor Author

arnb-smnta commented May 2, 2024

Run In Postman Postman API's for testing

@jwala-anirudh
Copy link
Collaborator

@jwala-anirudh its added can u just let me know if both the problems are addressed properly ?

Why are you regenerating the yarn.lock file again? Just revert it back to it original format of what is present on master branch, the one which you have added comes with multiple security issues - it will reduce the reliability & we need to redo entire work. Cannot merge those changes

@arnb-smnta
Copy link
Contributor Author

arnb-smnta commented May 2, 2024

@jwala-anirudh i think it is done now can u confirm the yarn.lock file problem

Copy link
Collaborator

@wajeshubham wajeshubham left a comment

Choose a reason for hiding this comment

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

@arnb-smnta Kindly go through the review comments. @jwala-anirudh kindly review the architecture and functionality implemented in this app and check if it is correct once the review comments are resolved. Thank you!

src/constants.js Show resolved Hide resolved
src/models/apps/expense-split-app/expense.model.js Outdated Show resolved Hide resolved
src/models/apps/expense-split-app/expense.model.js Outdated Show resolved Hide resolved
src/models/apps/expense-split-app/expense.model.js Outdated Show resolved Hide resolved
src/models/apps/expense-split-app/expensegroup.model.js Outdated Show resolved Hide resolved
src/models/apps/expense-split-app/settlement.model.js Outdated Show resolved Hide resolved
src/routes/apps/expense-split-app/expense.routes.js Outdated Show resolved Hide resolved
src/validators/apps/expense-split-app/expense.validator.js Outdated Show resolved Hide resolved
src/routes/apps/expense-split-app/expense.routes.js Outdated Show resolved Hide resolved
@arnb-smnta
Copy link
Contributor Author

Run In Postman @jwala-anirudh this is the new postman api collection corected the changes suggested by shubham

@arnb-smnta
Copy link
Contributor Author

arnb-smnta commented May 5, 2024

@wajeshubham commited the changes suggested by you just not deleted the route comments for my understanding ,I was thinking of deleting the route comments just before the final commit after all testing and approval has been done for merging will it be okay if I just leave the comments for now will surely delete before the final review

Copy link

🔗Pullpo.io Slack channel

@arnb-smnta
Copy link
Contributor Author

@jwala-anirudh sir any update on this

@arnb-smnta
Copy link
Contributor Author

@wajeshubham @jwala-anirudh sir any update on this

@wajeshubham
Copy link
Collaborator

@arnb-smnta Please go through all the review comments and fix those. If you have any doubts regarding any comment, please reply to the comment. Thank you.

@arnb-smnta
Copy link
Contributor Author

@jwala-anirudh @wajeshubham any updates on this

@arnb-smnta
Copy link
Contributor Author

@jwala-anirudh @wajeshubham any updates on this

@arnb-smnta
Copy link
Contributor Author

@wajeshubham @jwala-anirudh I have resolved all the issues as stated and the API is working fine can you just go through the code if any modification is required or not

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.

3 participants