Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating orders.html, presenting orders history #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Hotails/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
"""
from django.contrib import admin
from django.urls import path

import orders.views
from main import views
from django.contrib.auth import views as auth_views

Expand All @@ -26,4 +28,5 @@
path('homepage/', views.homepage, name='homepage'),
path('logout/', views.logout_view, name='logout'),
path('about/', views.about, name='about'),
path('orders/', orders.views.orders, name='orders'),
]
2 changes: 1 addition & 1 deletion main/templates/main/base_template.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
<ul class="navbar-nav mr-auto">
{% if request.user.is_authenticated %}
<li class="nav-item"><a class="nav-link text-white" href="/profile">Profile</a></li>
<li class="nav-item"><a class="nav-link text-white" href="">Orders</a></li>
<li class="nav-item"><a class="nav-link text-white" href="/orders">Orders</a></li>
<li class="nav-item"><a class="nav-link text-white" href="">Search</a></li>
<li class="nav-item"><a class="nav-link text-white" href="">Chats</a></li>
{% endif %}
Expand Down
26 changes: 26 additions & 0 deletions main/tests.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from orders.models import Order
from daycare.models import DayCare


Expand Down Expand Up @@ -77,3 +78,28 @@ def test_dog_owner_homepage_is_visible_for_dog_owner(self, client, create_dog_ow
response = client.get("/homepage/")
assert response.status_code == 200
assert list(response.context['daycares']) == list(DayCare.objects.all())


@pytest.mark.django_db
class TestOrdersView:
OfirMatasas marked this conversation as resolved.
Show resolved Hide resolved
def test_only_orders_which_belongs_to_user_as_daycare_are_displayed(self, client, create_daycare_user):
client.force_login(user=create_daycare_user.user)
response = client.get("/orders/")

response_orders_list = list(response.context['orders'])
all_orders_of_daycare_list = list(Order.objects.filter(daycare_id=create_daycare_user))
Omeramsc marked this conversation as resolved.
Show resolved Hide resolved
response_user = response.context['user']

assert response_user == 'daycare'
Omeramsc marked this conversation as resolved.
Show resolved Hide resolved
assert response_orders_list == all_orders_of_daycare_list

def test_only_orders_which_belongs_to_user_as_dog_owner_are_displayed(self, client, create_dog_owner_user):
client.force_login(user=create_dog_owner_user.user)
response = client.get("/orders/")

response_orders_list = list(response.context['orders'])
all_orders_of_dog_owner_list = list(Order.objects.filter(dog_owner_id=create_dog_owner_user))
response_user = response.context['user']

assert response_user == 'dog_owner'
Omeramsc marked this conversation as resolved.
Show resolved Hide resolved
assert response_orders_list == all_orders_of_dog_owner_list
49 changes: 49 additions & 0 deletions orders/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from django.utils import timezone
from daycare.models import DayCare
from dogowner.models import DogOwner
import datetime


class StatusOptions(models.TextChoices):
Expand Down Expand Up @@ -71,3 +72,51 @@ def cancel_order(self):

def get_order_total_price(self):
return self.price_per_day * (self.end_date - self.start_date).days

def get_order_status(self):
OfirMatasas marked this conversation as resolved.
Show resolved Hide resolved
if self.status == StatusOptions.Pending:
return "Pending"
elif self.status == StatusOptions.Approved:
return "Approved"
elif self.status == StatusOptions.Canceled:
return "Canceled"
elif self.status == StatusOptions.OnGoing:
return "Ongoing"
return "Finished"
Comment on lines +76 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified to

statusOptionsNameIdx=1
reuturn self.status[statusOptionsNameIdx]

WDYT?


@staticmethod
def get_capacity_of_daycare_in_dates_range(daycare_id: DayCare, start_datetime: datetime, end_datetime: datetime):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is capacity?

number_of_days = (end_datetime - start_datetime).days
capacity_per_day: list[int] = [0] * number_of_days
start_date = datetime.date(year=start_datetime.year, month=start_datetime.month, day=start_datetime.day)
end_date = datetime.date(year=end_datetime.year, month=end_datetime.month, day=end_datetime.day)
relevant_orders = Order.objects.filter(daycare_id=daycare_id,
status__in=['A', 'O'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use full names?

end_date__gte=start_date,
start_date__lte=end_date)

for order in relevant_orders:
for day in range(number_of_days):
current_date = order.start_date + datetime.timedelta(days=day)
if current_date < start_date:
continue
elif current_date > end_date:
break
else:
capacity_per_day[day] = capacity_per_day[day] + 1

return capacity_per_day

def are_order_dates_available(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made "test_order_is_approvable_according_to_daycare_capacity" for this method. Should I test it in another way?

Copy link
Contributor

@Omeramsc Omeramsc May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it testing this function alone?
if it does, it deserves a name change :)

Otherwise, we need a test to test this only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing this function does is check if specific dates are available due to capacity. the test you wrote deal with the whole process your application does. What we need for this function is a unit test, as it tests this unit specifically, without dependencies from other processes.

capacity_per_day: list[int] = self.get_capacity_of_daycare_in_dates_range(self.daycare_id,
self.start_date,
self.end_date)

daycare_capacity = DayCare.objects.get(name=self.daycare_id.name).capacity
return all(current_capacity < daycare_capacity for current_capacity in capacity_per_day)

def is_the_order_cancelable(self):
return self.status == StatusOptions.Pending or self.status == StatusOptions.Approved

def is_the_order_approvable(self):
return self.status == StatusOptions.Pending and self.are_order_dates_available()
54 changes: 54 additions & 0 deletions orders/templates/orders/orders.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
{% extends "main/base_template.html" %}
{% load static %}

{% block stylesheets %}
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-EVSTQN3/azprG1Anm3QDgpJLIm9Nao0Yz1ztcQTwFspd3yD65VohhpuuCOmLASjC" crossorigin="anonymous">
<link rel="stylesheet" type="text/css" href="{% static 'css/orders.css' %}">
{% endblock %}

{% block content %}
<div class="container">
<div class="row">
<div class="col-12">
<table class="table table-bordered">
<thead>
<tr>
<th scope="col">Name</th>
<th scope="col">Book Date</th>
<th scope="col">Start Date</th>
<th scope="col">End Date</th>
<th scope="col">Status</th>
<th scope="col">Actions</th>
</tr>
</thead>

<tbody>
{% for order in orders %}
<tr>
<th scope="row">
{% if user == 'daycare' %}
{{ order.dog_owner_id }}
{% else %}
{{ order.daycare_id.name }}
{% endif %}
</th>
<td>{{order.book_date}}</td>
<td>{{order.start_date}}</td>
<td>{{order.end_date}}</td>
<td>{{order.get_order_status}}</td>
<td>
{% if order.is_the_order_approvable and user == 'daycare' %}
<button type="button" class="btn btn-success">Approve</button>
{% endif %}
OfirMatasas marked this conversation as resolved.
Show resolved Hide resolved
{% if order.is_the_order_cancelable %}
<button type="button" class="btn btn-danger">Cancel</button>
{% endif %}
</td>
</tr>
{% endfor %}
</tbody>
</table>
</div>
</div>
</div>
{% endblock %}
63 changes: 63 additions & 0 deletions orders/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,66 @@ def test_dog_owner_id_is_deleted_when_dog_owner_is_deleted(self, create_order):
def test_daycare_id_is_deleted_when_daycare_is_deleted(self, create_order):
DayCare.objects.get(id=1).delete()
assert Order.objects.get(id=create_order.id).daycare_id is None

@pytest.mark.parametrize("dog_owner_1_id, dog_owner_2_id, delta_1, delta_2, capacity, expected_result",
[(1, 2, 3, 5, 1, False),
(2, 5, 4, 3, 10, True),
(5, 4, 6, 7, 15, True),
(4, 3, 4, 2, 1, False)])
def test_order_is_approvable_according_to_daycare_capacity(self,
dog_owner_1_id: DogOwner,
dog_owner_2_id: DogOwner,
delta_1: int,
delta_2: int,
capacity: int,
expected_result: bool):
daycare = DayCare.create("[email protected]", "CapacityUserName", "CapacityPassword", "CapacityName",
"Changeable capacity", 100, capacity, "Merkaz", "Tel Aviv", "Capacity 123")
dog_owner_1 = DogOwner.objects.get(id=dog_owner_1_id)
dog_owner_2 = DogOwner.objects.get(id=dog_owner_2_id)

order1 = Order.create(start_date=timezone.now(), end_date=timezone.now() + datetime.timedelta(days=delta_1),
daycare_id=daycare, dog_owner_id=dog_owner_1, price_per_day=100)
order2 = Order.create(start_date=timezone.now(), end_date=timezone.now() + datetime.timedelta(days=delta_2),
daycare_id=daycare, dog_owner_id=dog_owner_2, price_per_day=100)

order1.approve_order()
assert order2.is_the_order_approvable() == expected_result

@pytest.mark.parametrize("new_status, expected_result", [(StatusOptions.Pending, True),
(StatusOptions.Canceled, False),
(StatusOptions.Approved, True),
(StatusOptions.OnGoing, False),
(StatusOptions.Finished, False)])
def test_order_cancellation_according_to_order_status(self,
create_order: Order,
new_status: StatusOptions,
expected_result: StatusOptions):
create_order.status = new_status
assert create_order.is_the_order_cancelable() == expected_result

@pytest.mark.parametrize("new_status, expected_result", [(StatusOptions.Pending, "Pending"),
(StatusOptions.Canceled, "Canceled"),
(StatusOptions.Approved, "Approved"),
(StatusOptions.OnGoing, "Ongoing"),
(StatusOptions.Finished, "Finished")])
def test_get_order_status_as_string(self,
create_order: Order,
new_status: StatusOptions,
expected_result: StatusOptions):
create_order.status = new_status
assert create_order.get_order_status() == expected_result

def test_get_daycare_capacity_in_dates_range(self, create_order):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how you should test this function. it has a lot of logic; you need to test different cases and see that you get the capacity you expect in each case

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that checking the current capacity of a daycare before and after approving an order will check if the method is working correctly.
Also, some of the other tests are testing this method too (like "test_order_is_approvable_according_to_daycare_capacity").

Do you have any example for a case that I should test for this method? Or another way for testing it?

Copy link
Contributor

@Omeramsc Omeramsc May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first of all - test names should follow the method they're testing (but not only the method name).
as an example:
if the method name is: check_capacity_of_daycare
possible test names are:
test_check_capacity_of_daycare
test_check_capacity_of_daycare_raise_value_error_when_something_happends
test_check_capacity_of_daycare_reduced_after_an_order
etc.

otherwise, it is very confusing to understand what is testing what.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second:
the test above test if order can be approved, according to its overall calculated capacity
this test should be a specific unit-test for get_capacity_of_daycare_in_dates_range it doesn't test the overall logic your app will go through, but it is testing this method specifically, and should pass even if in the future you'll decide to change the way the process works.

this method has a lot of cases. think of each if statement as at least two possible test-cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OfirMatasas are you going to address these?

original_capacity_list = Order.get_capacity_of_daycare_in_dates_range(create_order.daycare_id,
create_order.start_date,
create_order.end_date)
create_order.approve_order()
updated_capacity_list = Order.get_capacity_of_daycare_in_dates_range(create_order.daycare_id,
create_order.start_date,
create_order.end_date)
assert len(original_capacity_list) == len(updated_capacity_list)

equality_list = [updated_capacity_list[i] == original_capacity_list[i] + 1
for i in range(len(original_capacity_list))]
assert equality_list.count(True) == len(equality_list)
22 changes: 20 additions & 2 deletions orders/views.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
# from django.shortcuts import render
from django.contrib.auth.decorators import login_required
from django.shortcuts import render

# Create your views here.
from dogowner.models import DogOwner
from orders.models import Order


@login_required()
def orders(request):
if DogOwner.objects.filter(user=request.user).exists():
context = {
'orders': Order.objects.filter(dog_owner_id__user=request.user),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to fillter by authenticated user, is it possible to use request.user directly?

'user': 'dog_owner',
}
else:
context = {
'orders': Order.objects.filter(daycare_id__user=request.user),
'user': 'daycare',
}

return render(request, 'orders/orders.html', context)
8 changes: 8 additions & 0 deletions static/CSS/orders.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
td, th
{
text-align: center;
}

.container {
padding: 2rem 0;
}