#2 sprint-4

Ouvert
paras veut fusionner 7 révision(s) depuis sprint-4 vers main
paras a commenté il y a 2 ans

Added .gitignore and cleared main branch for a full review

Added .gitignore and cleared main branch for a full review
paras demande de révision de kj il y a 2 ans
paras a supprimé la demande de révision pour kj il y a 2 ans
paras demande de révision de kj il y a 2 ans
kj a commenté il y a 2 ans

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 révisé il y a 2 ans
kj laisser un commentaire

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 révisé il y a 2 ans
kj laisser un commentaire

Remove Readme.md

It’s a blank useless file

kj révisé il y a 2 ans
kj laisser un commentaire

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

kj a requis les changements il y a 2 ans
@@ -0,0 +4,4 @@
class AccountForm(forms.ModelForm):
class Meta:
model = Account
exclude = () #
kj a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

Useless line

Useless line
@@ -0,0 +7,4 @@
class CompanyViewSet(viewsets.ModelViewSet):
queryset = Company.objects.all()
serializer_class = CompanySerializer
permission_classes = [permissions.IsAuthenticated]
kj a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 a commenté il y a 2 ans

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 demande de révision de navaneeth il y a 2 ans
paras demande de révision de kj il y a 2 ans
Ce dépôt est archivé. Vous ne pouvez pas commenter de demande d'ajout.
Pas de relecteur
Pas d'étiquette
Aucun jalon
Pas d'assignataires
3 participants
Échéance

Aucune échéance n'a été définie.

Dépendances

La demande de fusion n'a actuellement pas de dépendance.

Chargement…
Il n'existe pas encore de contenu.