#2 sprint-4

Abierta
paras desea fusionar 7 commits de sprint-4 en main
paras comentado hace 2 años

Added .gitignore and cleared main branch for a full review

Added .gitignore and cleared main branch for a full review
paras solicitud de revisión de kj hace 2 años
paras solicitud de revisión eliminada para kj hace 2 años
paras solicitud de revisión de kj hace 2 años
kj comentado hace 2 años

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.

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.
kj revisado hace 2 años
kj dejó un comentario

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 revisado hace 2 años
kj dejó un comentario

Remove Readme.md

It’s a blank useless file

kj revisado hace 2 años
kj dejó un comentario

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

kj cambios solicitados hace 2 años
@@ -0,0 +4,4 @@
class AccountForm(forms.ModelForm):
class Meta:
model = Account
exclude = () #
kj comentado hace 2 años

Useless line, please remomve it.

Useless line, please remomve it.
@@ -0,0 +6,4 @@

class Client(models.Model):
name = models.CharField(max_length=100)
# Other fields for client information
kj comentado hace 2 años

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

Please revert back.

Seems like thie model is not complete. Was this intentional? Please revert back.
@@ -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 comentado hace 2 años

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.
@@ -0,0 +47,4 @@
# Other expense categories
]

category = models.CharField(max_length=100, choices=CATEGORY_CHOICES)
kj comentado hace 2 años

Missing VALUE/AMO0UNT of Expense field.

Missing VALUE/AMO0UNT of Expense field.
@@ -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 comentado hace 2 años

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

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
@@ -0,0 +6,4 @@
admin.site.register(Client)
admin.site.register(Contract)
admin.site.register(Requirement)
admin.site.register(Project)
kj comentado hace 2 años

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

Shorten this code

All Admin side Registration can be done in one Shot using Array format. Shorten this code
@@ -0,0 +26,4 @@

class Requirement(models.Model):
name = models.CharField(max_length=255)
description = models.TextField()
kj comentado hace 2 años

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

Add attachment / file field since there are high chances of uploading the document as requirement rather than typing it inside the description field.
@@ -0,0 +5,4 @@

admin.site.register(Address)
admin.site.register(BankDetails)
admin.site.register(NatureOfBusiness)
kj comentado hace 2 años

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

As mentioned earlier, make this a One Liner statement by registering in Array format
@@ -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 comentado hace 2 años

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

Add Lat,Lng since we can also get the location name by the coordinates. They can be made optional.
@@ -0,0 +1,3 @@
from django.test import TestCase

# Create your tests here.
kj comentado hace 2 años

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

Remove the file if it's not gonna be useful.
@@ -0,0 +4,4 @@
class CompanyForm(forms.ModelForm):
class Meta:
model = Company
exclude = () #
kj comentado hace 2 años

Useless line

Useless line
@@ -0,0 +7,4 @@
class CompanyViewSet(viewsets.ModelViewSet):
queryset = Company.objects.all()
serializer_class = CompanySerializer
permission_classes = [permissions.IsAuthenticated]
kj comentado hace 2 años

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

Is authenticated is not a good enough security especially to manipulate Employee data. Please do the needful.
@@ -0,0 +4,4 @@

class Employee(models.Model):
user = models.OneToOneField(User, on_delete=models.CASCADE, related_name='employee')
# Rest of the fields...
kj comentado hace 2 años

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

Employee ID has a pattern and should be generated on the fly or during the creation of the data. Please add the field.
@@ -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 comentado hace 2 años

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

Can we add another field called Reporting Manager? Please check with Shashank.
@@ -0,0 +18,4 @@
user = request.user
if user.is_authenticated and user.role:
role_name = user.role.name
if role_name == 'Admin':
kj comentado hace 2 años

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 solicitud de revisión de navaneeth hace 2 años
paras solicitud de revisión de kj hace 2 años
Este repositorio está archivado. No se puede comentar en los pull requests.
No hay revisores
Sin etiquetas
Sin Milestone
No asignados
3 participantes
Fecha de vencimiento

Sin fecha de vencimiento.

Dependencias

Este pull request actualmente no tiene ninguna dependencia.

Cargando…
Aún no existe contenido.