sprint-4

Added .gitignore and cleared main branch for a full review

paras heeft een review aangevraagd van kj 2 jaren geleden

paras beoordelingsaanvraag voor kj 2 jaren geleden verwijderd

paras heeft een review aangevraagd van kj 2 jaren geleden

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 beoordeeld 2 jaren geleden

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 beoordeeld 2 jaren geleden

Remove Readme.md

It’s a blank useless file

kj beoordeeld 2 jaren geleden

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

kj aangevraagde wijzigingen 2 jaren geleden

workx/accounts/form.py

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

kj commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 commented 2 jaren geleden

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 heeft een review aangevraagd van navaneeth 2 jaren geleden

paras heeft een review aangevraagd van kj 2 jaren geleden

Deze repo is gearchiveerd. U kunt niet reageren op pull requests.

navaneeth

Geen label

Geen mijlpaal

Niet toegewezen

3 deelnemers

Vervaldatum

Geen vervaldatum ingesteld.

Afhankelijkheden

Deze pull-aanvraag heeft momenteel geen afhankelijkheden.

#2 sprint-4