Review

[CatalinAtanase]

Hello, thank you for doing this task. I will add some notes and please make a pull request for this.

The app name should usually be the main model name but using plural eg reservations. For this one I think we can continue with your app name.

admin.py

• Admin plural name -> Reservationss
• Please remove the comment as well

  forms.py

• if validators import is not used please remove it
• when you set the choices, values is not used

  models.py

• Please use singular name for model name
• Please use decimalField for representing money
• Also the field reservation on the Reservation model is strange -> in code will be reservation.reservation, maybe a better name can be used

  views copy.py

• This file can be removed I think

  views.py

• from django.contrib import messages is not used
• constant vars should be moved in a separate file
• we use class based views, please rewrite this logic using class based views
• if something doesn't work on the report page user will get redirected to the error page with a wrong message for the case

• upload:

  • why delete all the data?

  • check for post request

  • the logic for creating a reservation should be moved from views, maybe to some service

  • if you don't use any of the data from Reservations.objects.update_or_create there's no need to assign vars

  • for redirect please use view name

  • report:

  • please compute the total_commission using django ORM

  • no need to iterate over the dictionary, you can just check if the month is in the dictionary

  • also this query could be done usign django ORM as well and returning: month/year -> value

@Irostovsky

• city_commission
  • Reservations.objects.filter(city=form.cleaned_data["rates"]) city filter gets data from a var called rates, maybe city would be a better name
  • please compute commission_amount_city using Django ORM
  • remove print

templates:

• use url tag in templates for links

• report.html

  • Upload message shows even if I don't make an upload, maybe using django messages would be better
  • {{ report.checkin }} {{ report.checkout }} {{ report.flat }} I don't think this is used

about testing, django uses unittest already using a class based approach. see https://docs.djangoproject.com/en/4.0/topics/testing/overview/

cc @Irostovsky

[MirkoMilanovic]

Hello Catalin, thank you for your review and for the good notes given!

I did correct most stuff, but some ORM queries don't work as expected. So I left the old solution there for now. For example, I have tried something like this:

total_commission = Reservation.objects.filter(city__in=COMMISSION_RATES.keys()).annotate(instance_commission=F('net_income') * COMMISSION_RATES.get(F('city')) / 100).aggregate(total_commission=Sum('instance_commission'))['total_commission']

or many different variations like so, but it doesn't work. Maybe I can get some tips about that or some more time to investigate and work on that.

Thank you, and have a nice weekend! Mirko Milanovic

[MirkoMilanovic]

Hello @CatalinAtanase I did upload the solution afterwards. It seems way better and more professional now!

Thank you and Happy Easter! Mirko Milanovic