First of all, the PR name makes no sense. Please mention what the PR is raised for?
Below is the pattern you can follow:
Title
Description / Purpose behind this PR
Any references / Attachments we should keep
This helps the reviewer understad the intent and judge better.
First of all, the PR name makes no sense. Please mention what the PR is raised for?
Below is the pattern you can follow:
1. Title
2. Description / Purpose behind this PR
3. Any references / Attachments we should keep
This helps the reviewer understad the intent and judge better.
Business Take:
Does the invoice include CGST, IGST breakup? Is it calculated implicitly?
Please add the field if missing.
Also, I don’t see THE CURRENCY and VALUE of the INCOME.
Business Take:
Does the invoice include CGST, IGST breakup? Is it calculated implicitly?
Please add the field if missing.
Also, I don't see THE CURRENCY and VALUE of the INCOME.
Also, was the reimbursment paid or not? No flag for that?
If Expense / Reimbursement / Income invoice record generated has to be deleted it shouldn’t undergo delete operation.
Please add ISARCHIVED Boolean field
What is the boolean used for?
Also, was the reimbursment paid or not? No flag for that?
If Expense / Reimbursement / Income invoice record generated has to be deleted it shouldn't undergo delete operation.
Please add ISARCHIVED Boolean field
Checking against the word “Admin” for permission is not the right way.
Please use Choices or Enum and make the Roles to be common in such a way that there is no ambiguity in checking the User Role.
Checking against the word "Admin" for permission is not the right way.
Please use Choices or Enum and make the Roles to be common in such a way that there is no ambiguity in checking the User Role.
paras
requested review from navaneeth 2 anos atrás
Added .gitignore and cleared main branch for a full review
First of all, the PR name makes no sense. Please mention what the PR is raised for?
Below is the pattern you can follow:
This helps the reviewer understad the intent and judge better.
I see two Pip Files
One in Root of the folder.
Another inside “workx/”
Please mention the reason behind this. If it’s useless, please remove the Pipfile which is present inside “workx”
Remove Readme.md
It’s a blank useless file
The Piplock file is not needed as far as I know. Remove them in both root and inside “workx/”
Useless line, please remomve it.
Seems like thie model is not complete. Was this intentional?
Please revert back.
Business Take:
Does the invoice include CGST, IGST breakup? Is it calculated implicitly?
Please add the field if missing.
Also, I don’t see THE CURRENCY and VALUE of the INCOME.
Missing VALUE/AMO0UNT of Expense field.
What is the boolean used for?
Also, was the reimbursment paid or not? No flag for that?
If Expense / Reimbursement / Income invoice record generated has to be deleted it shouldn’t undergo delete operation.
Please add ISARCHIVED Boolean field
All Admin side Registration can be done in one Shot using Array format.
Shorten this code
Add attachment / file field since there are high chances of uploading the document as requirement rather than typing it inside the description field.
As mentioned earlier, make this a One Liner statement by registering in Array format
Add Lat,Lng since we can also get the location name by the coordinates. They can be made optional.
Remove the file if it’s not gonna be useful.
Useless line
Is authenticated is not a good enough security especially to manipulate Employee data. Please do the needful.
Employee ID has a pattern and should be generated on the fly or during the creation of the data. Please add the field.
Can we add another field called Reporting Manager? Please check with Shashank.
Checking against the word “Admin” for permission is not the right way.
Please use Choices or Enum and make the Roles to be common in such a way that there is no ambiguity in checking the User Role.