#2 sprint-4

Atvērta
paras vēlas sapludināt 7 revīzijas no sprint-4 uz main
paras komentēja pirms 2 gadiem

Added .gitignore and cleared main branch for a full review

Added .gitignore and cleared main branch for a full review
paras pieprasīja recenziju no kj pirms 2 gadiem
paras noņema recenzijas pieprasījumu no kj pirms 2 gadiem
paras pieprasīja recenziju no kj pirms 2 gadiem
kj komentēja pirms 2 gadiem

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 recenzēja pirms 2 gadiem
kj atstāja komentāru

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 recenzēja pirms 2 gadiem
kj atstāja komentāru

Remove Readme.md

It’s a blank useless file

kj recenzēja pirms 2 gadiem
kj atstāja komentāru

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

kj pieprasīja izmaiņas pirms 2 gadiem
@@ -0,0 +4,4 @@
class AccountForm(forms.ModelForm):
class Meta:
model = Account
exclude = () #
kj komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

Useless line

Useless line
@@ -0,0 +7,4 @@
class CompanyViewSet(viewsets.ModelViewSet):
queryset = Company.objects.all()
serializer_class = CompanySerializer
permission_classes = [permissions.IsAuthenticated]
kj komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 komentēja pirms 2 gadiem

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 pieprasīja recenziju no navaneeth pirms 2 gadiem
paras pieprasīja recenziju no kj pirms 2 gadiem
Repozitorijs ir arhivēts. Izmaiņu pieprasījumiem nevar pievienot jaunus komentārus.
Nav recenzentu
Nav etiķešu
Nav atskaites punktu
Nav atbildīgo
3 dalībnieki
Izpildes termiņš

Izpildes termiņš nav uzstādīts.

Atkarības

Šim izmaiņu pieprasījumam pagaidām nav nevienas atkarības.

Notiek ielāde…
Vēl nav satura.