#2 sprint-4

Aperto
paras vorrebbe unire 7 commit da sprint-4 a main
paras 2 anni fa ha commentato

Added .gitignore and cleared main branch for a full review

Added .gitignore and cleared main branch for a full review
paras requested review from kj 2 anni fa
paras removed review request for kj 2 anni fa
paras requested review from kj 2 anni fa
kj 2 anni fa ha commentato

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 revisionato 2 anni fa
kj lascia un commento

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 revisionato 2 anni fa
kj lascia un commento

Remove Readme.md

It’s a blank useless file

kj revisionato 2 anni fa
kj lascia un commento

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

kj richieste modifiche 2 anni fa
@@ -0,0 +4,4 @@
class AccountForm(forms.ModelForm):
class Meta:
model = Account
exclude = () #
kj 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

Useless line

Useless line
@@ -0,0 +7,4 @@
class CompanyViewSet(viewsets.ModelViewSet):
queryset = Company.objects.all()
serializer_class = CompanySerializer
permission_classes = [permissions.IsAuthenticated]
kj 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 2 anni fa ha commentato

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 anni fa
paras requested review from kj 2 anni fa
Questo repository è archiviato. Non puoi commentare le richieste di pull.
Nessun revisore
Nessuna etichetta
Nessuna milestone
Nessuna assegnatario
3 Partecipanti
Data di scadenza

Nessuna data di scadenza impostata.

Dipendenze

Questo problema attualmente non ha alcuna dipendenza.

Caricamento…
Non ci sono ancora contenuti.