sprint-4

Added .gitignore and cleared main branch for a full review

paras 请求 kj 评审 2 年前

paras 取消对 kj 的评审请求 2 年前

paras 请求 kj 评审 2 年前

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.

kj 评审于 2 年前

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”

kj 评审于 2 年前

Remove Readme.md

It’s a blank useless file

kj 评审于 2 年前

The Piplock file is not needed as far as I know. Remove them in both root and inside “workx/”

kj 请求变更 2 年前

workx/accounts/form.py

												
			@@ -0,0 +4,4 @@
		
			class AccountForm(forms.ModelForm):
		
			    class Meta:
		
			        model = Account
		
			        exclude = ()  #

kj 评论于 2 年前

Useless line, please remomve it.

workx/accounts/models.py

												
			@@ -0,0 +6,4 @@
		
			class Client(models.Model):
		
			    name = models.CharField(max_length=100)
		
			    # Other fields for client information

kj 评论于 2 年前

Seems like thie model is not complete. Was this intentional?

Please revert back.

workx/accounts/models.py

												
			@@ -0,0 +32,4 @@
		
			    date = models.DateField()
		
			    client = models.ForeignKey(Client, on_delete=models.CASCADE)
		
			    project = models.ForeignKey(Project, on_delete=models.CASCADE)
		
			    # Other fields specific to income

kj 评论于 2 年前

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.

workx/accounts/models.py

												
			@@ -0,0 +47,4 @@
		
			        # Other expense categories
		
			    ]
		
			    category = models.CharField(max_length=100, choices=CATEGORY_CHOICES)

kj 评论于 2 年前

Missing VALUE/AMO0UNT of Expense field.

workx/accounts/models.py

												
			@@ -0,0 +53,4 @@
		
			    date = models.DateField()
		
			    employee = models.ForeignKey(Employee, on_delete=models.CASCADE)
		
			    bank_account = models.ForeignKey(BankDetails, on_delete=models.CASCADE)
		
			    reimbursement = models.BooleanField()

kj 评论于 2 年前

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

workx/client/admin.py

												
			@@ -0,0 +6,4 @@
		
			admin.site.register(Client)
		
			admin.site.register(Contract)
		
			admin.site.register(Requirement)
		
			admin.site.register(Project)

kj 评论于 2 年前

All Admin side Registration can be done in one Shot using Array format.

Shorten this code

workx/client/models.py

												
			@@ -0,0 +26,4 @@
		
			class Requirement(models.Model):
		
			    name = models.CharField(max_length=255)
		
			    description = models.TextField()

kj 评论于 2 年前

Add attachment / file field since there are high chances of uploading the document as requirement rather than typing it inside the description field.

workx/common/admin.py

												
			@@ -0,0 +5,4 @@
		
			admin.site.register(Address)
		
			admin.site.register(BankDetails)
		
			admin.site.register(NatureOfBusiness)

kj 评论于 2 年前

As mentioned earlier, make this a One Liner statement by registering in Array format

workx/common/models.py

												
			@@ -0,0 +8,4 @@
		
			    city = models.CharField(max_length=50)
		
			    state = models.CharField(max_length=50)
		
			    zip_code = models.CharField(max_length=10)
		
			    country = models.CharField(max_length=50)

kj 评论于 2 年前

Add Lat,Lng since we can also get the location name by the coordinates. They can be made optional.

workx/common/tests.py

												
			@@ -0,0 +1,3 @@
		
			from django.test import TestCase
		
			# Create your tests here.

kj 评论于 2 年前

Remove the file if it’s not gonna be useful.

workx/company/form.py

												
			@@ -0,0 +4,4 @@
		
			class CompanyForm(forms.ModelForm):
		
			    class Meta:
		
			        model = Company
		
			        exclude = ()  #

kj 评论于 2 年前

Useless line

workx/company/views.py

												
			@@ -0,0 +7,4 @@
		
			class CompanyViewSet(viewsets.ModelViewSet):
		
			    queryset = Company.objects.all()
		
			    serializer_class = CompanySerializer
		
			    permission_classes = [permissions.IsAuthenticated]

kj 评论于 2 年前

Is authenticated is not a good enough security especially to manipulate Employee data. Please do the needful.

workx/employee/models.py

												
			@@ -0,0 +4,4 @@
		
			class Employee(models.Model):
		
			    user = models.OneToOneField(User, on_delete=models.CASCADE, related_name='employee')
		
			    # Rest of the fields...

kj 评论于 2 年前

Employee ID has a pattern and should be generated on the fly or during the creation of the data. Please add the field.

workx/employee/models.py

												
			@@ -0,0 +15,4 @@
		
			    job_title = models.CharField(max_length=100)
		
			    ctc = models.DecimalField(max_digits=10, decimal_places=2)
		
			    bank_details = models.OneToOneField(BankDetails, on_delete=models.CASCADE)

kj 评论于 2 年前

Can we add another field called Reporting Manager? Please check with Shashank.

workx/employee/permissions.py

												
			@@ -0,0 +18,4 @@
		
			        user = request.user
		
			        if user.is_authenticated and user.role:
		
			            role_name = user.role.name
		
			            if role_name == 'Admin':

kj 评论于 2 年前

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 请求 navaneeth 评审 2 年前

paras 请求 kj 评审 2 年前

此仓库已存档，您不能在此合并请求添加评论。

navaneeth

未选择标签

未选择里程碑

未指派成员

3 名参与者

到期时间

未设置到期时间。

依赖工单

此合并请求当前没有任何依赖。

#2 sprint-4