#2 sprint-4

Открыто
paras хочет смерджить 7 коммит(ов) из sprint-4 в main
paras прокомментировал 2 лет назад

Added .gitignore and cleared main branch for a full review

Added .gitignore and cleared main branch for a full review
paras запросил отзыв от kj 2 лет назад
paras удалена заявка на отзыв для kj 2 лет назад
paras запросил отзыв от kj 2 лет назад
kj прокомментировал 2 лет назад

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 рассмотрел(а) изменения 2 лет назад
kj оставил комментарий

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 рассмотрел(а) изменения 2 лет назад
kj оставил комментарий

Remove Readme.md

It’s a blank useless file

kj рассмотрел(а) изменения 2 лет назад
kj оставил комментарий

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

kj запросил(а) изменения 2 лет назад
@@ -0,0 +4,4 @@
class AccountForm(forms.ModelForm):
class Meta:
model = Account
exclude = () #
kj прокомментировал 2 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 лет назад

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 запросил отзыв от navaneeth 2 лет назад
paras запросил отзыв от kj 2 лет назад
Это архивный репозиторий. Вы не можете комментировать запросы на слияние.
Нет рецензентов
Нет меток
Нет этапа
Нет назначенных лиц
3 участников
Срок выполнения

Срок выполнения не установлен.

Зависимости

Этот запрос на слияние в настоящее время не имеет никаких зависимостей.

Загрузка…
Пока нет содержимого.