Add search day-care functionality to dog-owner homepage. #89

tamirmatok · 2022-04-27T16:09:04Z

Desired Outcome

The dog-owner user will search for day-care that relevant for him.
If the user didnt search for day-care he will see all Day-Cares in dog-owner homepage.

Implemented Changes

Adding search functionality to dog-owner homepage.
Adding "area" choices-class to day-care model for make the data model compatible and to improve the search.
Delete "search" button from navbar.

Connected Issue/Story

depends on #95
Closes #90

Test coverage

This PR includes new unit and integration tests to go with the code
changes
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: insert issue ID
This PR does not require updating any documentation

daycare/migrations/0006_alter_daycare_area.py

Yuval-Vino · 2022-05-02T19:16:23Z

daycare/test_image.py

+    def test_daycare_has_default_profile_image(self, create_daycare_user):
+        daycare_profile_image = create_daycare_user.get_daycare_primary_image_url()
+        assert daycare_profile_image == DEFAULT_DAYCARE_PROFILE_URL
+        assert daycare_profile_image is not None


I think this test is redundant , if profile_image = default so obviously its not None

please note that this comment related to #67

Yuval-Vino · 2022-05-02T19:18:46Z

main/tests.py

+        client.force_login(user=create_dog_owner_user.user)
+        response = client.get("/homepage/")
+        day_care_queryset = response.context['day_care_queryset']
+        assert day_care_queryset.count() == DayCare.objects.all().count()


maybe instead of count use set(day_care_queryset) == set(daycare.objects.all) because if the count is the same we are not sure we presenting all daycares, maybe we present the same one 30 times?

Im not sure I fully understand , but in this test we want to check that day_care_queryset contain all Daycare objects when we are entering to dog_owner homepage.
Since every user is unique it is promise that every object in the queryset is diffrent from the other.

that's right for the right side of the assert (daycare.objects), what about what is presented in HTML (daycare_queryset), how can you be sure that they are unique and don't have dups, for example, daycare shown twice in HTML or things like that

And anyway it just seems weird to conclude equality between sets by their size, even if all the values are unique

The HTML running for loop and create a card for each daycare in day_care queryset,I think in this case we should test what this queryset contatin .
Thanks for the advice I fixed it to use set(day_care_queryset) == set(daycare.objects.all) and it is seems better !

Yuval-Vino · 2022-05-02T19:26:23Z

main/tests.py

+        for day_care in DayCare.objects.all():
+            if day_care.price_per_day <= 800 and day_care.area.lower() == 'center'\
+                    and day_care.city.lower() == 'tel aviv':
+                assert day_care in day_care_queryset


maybe also check that all day_care_queryset is in filter range,
your test checks that all filtered daycares are shown, maybe add a test that all daycares in day_care_queryset are the correct ones.
Easy hit append all daycares in the for to list, then assert equality between this list to day_care_queryset

You right . We can deal with it in the same way of #89 (comment)

ErezCohenn · 2022-05-03T05:10:50Z

@tamirmatok this PR is ready for review? notice that the PR is on draft

tamirmatok · 2022-05-03T10:09:40Z

@tamirmatok this PR is ready for review? notice that the PR is on draft

Now it is (:

OfirMatasas · 2022-05-03T22:55:35Z

The idea about making the area a choice box is great!
IMO, I think we should do the same with the cities, to avoid a situation where the daycare is on "Tel-Aviv", but the dog owner searched for "Tel aviv" or "Tel Aviv - Yaffo".
What do you think? @tamirmatok

ErezCohenn

What happens in the case that after filtering the daycares, there were no daycares that match the selection of the user? I think in that case we should write to the user something like:

There were no daycares found that match the filtering that was selected

tamirmatok · 2022-05-04T09:03:43Z

What happens in the case that after filtering the daycares, there were no daycares that match the selection of the user? I think in that case we should write to the user something like:

There were no daycares found that match the filtering that was selected

We have a generic sentence for all the search results : "Found %d results for your dog" . How does it sound ? @ErezCohenn

ErezCohenn · 2022-05-06T07:04:05Z

What happens in the case that after filtering the daycares, there were no daycares that match the selection of the user? I think in that case we should write to the user something like:

There were no daycares found that match the filtering that was selected

We have a generic sentence for all the search results : "Found %d results for your dog" . How does it sound ? @ErezCohenn

Sounds good :)

tamirmatok · 2022-05-07T16:18:38Z

The idea about making the area a choice box is great! IMO, I think we should do the same with the cities, to avoid a situation where the daycare is on "Tel-Aviv", but the dog owner searched for "Tel aviv" or "Tel Aviv - Yaffo". What do you think? @tamirmatok

Hi @OfirMatasas, I think you right and that issue appeared in my head as well.
Let me share with you my thoughts about it .
First, please note that "search" data fields/choices and "user-creation" fields/choices kind of come together(In our case search/create-user that involve city choices field).
I think that to add city a choice like you mention indeed will work in "static world" with no user-registration, but in that world, we can control the cities input and just avoid mistakes like that.

I think the right approch to deal with this issue is kind of "preapre the ground" for supporting in user-registration, with django-cities(see https://pypi.org/project/django-cities/), it looks better to me because of the benefits of global support or even local but with more percise data.
I believe we should meet as a team and to decide if we want to dedicate time on this kind of stuff at this point of the course.

OfirMatasas · 2022-05-09T19:50:09Z

The idea about making the area a choice box is great! IMO, I think we should do the same with the cities, to avoid a situation where the daycare is on "Tel-Aviv", but the dog owner searched for "Tel aviv" or "Tel Aviv - Yaffo". What do you think? @tamirmatok

Hi @OfirMatasas, I think you right and that issue appeared in my head as well. Let me share with you my thoughts about it . First, please note that "search" data fields/choices and "user-creation" fields/choices kind of come together(In our case search/create-user that involve city choices field). I think that to add city a choice like you mention indeed will work in "static world" with no user-registration, but in that world, we can control the cities input and just avoid mistakes like that.

I think the right approch to deal with this issue is kind of "preapre the ground" for supporting in user-registration, with django-cities(see https://pypi.org/project/django-cities/), it looks better to me because of the benefits of global support or even local but with more percise data. I believe we should meet as a team and to decide if we want to dedicate time on this kind of stuff at this point of the course.

I think you're right.
Besides that, switching the cities to choices will effect the rest of the project, so we'll discuss this on our next meeting.

ErezCohenn

LGTM

Omeramsc · 2022-05-11T15:04:03Z

daycare/forms.py

+
+class DayCareSearchForm(forms.Form):
+    min_price = DayCare.objects.all().aggregate(Min('price_per_day')).get('price_per_day__min')
+    max_price = DayCare.objects.all().aggregate(Max('price_per_day')).get('price_per_day__max')


I'm not sure the user should be aware of the min\max values a daycare can use for his pricing

I guess you right ,Fixed.

Omeramsc · 2022-05-11T15:05:04Z

daycare/forms.py

+
+    area = forms.ChoiceField(required=False, label='Area',
+                             choices=(Area.choices + [('', 'All'), ]), initial="")
+    city = forms.CharField(required=False, label='City')


Don't we have a list of cities? a free-text would be tough.

Please see #89 (comment) and let me know what you think.

Can you sum it up? I'm not sure I'm following. Anyway free-text can lead to issues (e.g: פתח-תקוה, פתח תקווה, פתח-תקווה, פתח תקוה)

List of cities seems to me good solution for static-users ,but because all users are static we can control the cities input and just avoid mistakes like that.
If we will want to support registration in the future it might limit us .
I saw django-cities(see https://pypi.org/project/django-cities/), it seems to have benefits of global support or even local but with more percise data.
I have no problem to create list of cities just want to know your opinion about it . @Omeramsc

The UI is an entry point for this app and as such it should perform validation on the input,
regardless if the users are static or not.
Nothing prevents for the next dev to create another static user and enter non valid city name, right?
If this is a lot of work you can do it on a follow up PR and write in the PR description about it.

Omeramsc · 2022-05-11T15:07:40Z

daycare/models.py

 class DayCare(models.Model):
    user = models.OneToOneField(User, on_delete=models.CASCADE, default=None, blank=True, null=False, editable=True)
    name = models.CharField(max_length=20, blank=True, unique=True, validators=[MaxLengthValidator])
    description = models.TextField(blank=True, null=True)
    price_per_day = models.IntegerField(blank=False, null=False, default=0)
    capacity = models.IntegerField(null=False, blank=True)
-    area = models.CharField(max_length=20, blank=True, validators=[MaxLengthValidator])
+    area = models.CharField(max_length=20, blank=True, validators=[MaxLengthValidator], choices=Area.choices)


Shouldn't we include that in some of the daycare fixtures?

Area field in create_day_care_user from the confest been updated.

Omeramsc · 2022-05-11T15:09:42Z

dogowner/views.py

+                                                                             form['end_date'].value())
+            day_care_queryset = filter_day_cares.intersection(available_day_cares)
+
+            context = {


no need to include it twice; since both use the same parameters, just include it once after the conditional statements.

You right NP, fixed.

Omeramsc · 2022-05-11T15:10:43Z

main/tests.py

+@pytest.mark.django_db
+class TestDogOwnerHomePageView:
+    def test_search_daycare_present_only_available_daycares_on_specific_dates(self, client, create_dog_owner_user):
+        test_day_care = DayCare.create(username='testuser', email='[email protected]', password='test_password',


didn't we add a bunch of fixtures to conftest so we won't need this anymore?

The reason I didnt use the fixture from the confest file is because it is easier to test this case through day-care with capcity=1.
Let me know if you think that it deserve a place in the confest file .

What is the difference? how much more work do you need to do if you use the fixture?

Just one for loop for decreasing the capcity from 50 to 0 (that means to create 50 orders to fulfill the capacity )
Another option is to change the capacity in the fixture . @Omeramsc

orders/models.py

Omeramsc · 2022-05-11T15:23:38Z

main/tests.py

+        filters_day_cares = DayCare.objects.filter(area__startswith='C',
+                                                   city__icontains="tel aviv",
+                                                   price_per_day__lte=800)
+        assert set(day_care_queryset) == set(available_day_cares.intersection(filters_day_cares))


Do we test the functionality of the filter? I mean: do we expect specific search results? what are the day cares that exists during this test?

The day-cares that exists during this test are the static day-cares from the migration file .
In this test I considered the filters queryset as the expected results and make sure that the 'day_care_queryset' from the response is the same .

The test should verify the functionality would work in any case, and not only a specific one we carefully created.

what is intersection? what does it provide? This proves the usefulness of declaring the variables before asserting them...

NP I will make it more clear .Since we want to filter by capacity of day-care in specific dates we have to create a query set that contain all day-cares avaible in dates , and the other queryset, with the other fields of the search.
The intersection between those querysets will give us all day-cares avaible in specific dates that fit with the other filters .

Do you have an idea how can I make this test more general like you mention ?
Because if I will try all the combination of the filters we won't get out of it. @Omeramsc

Omeramsc · 2022-05-11T15:24:24Z

main/tests.py

+        response = client.post('/homepage/', search_form, follow=True)
+        day_care_queryset = response.context['day_care_queryset']
+        assert test_day_care in day_care_queryset
+        Order.create(dog_owner_id=DogOwner.objects.get(id=1), daycare_id=test_day_care,


belong in a different test

Can you explain ? Do you think that create day-care with capcity=0, and test that it doesn't appear in the queryset is better approch ?

on the first assertion you check that a daycare with capacity is available,
in the second one you check that a daycare with no capacity is not available.
those are two different cases.
BUT, on second thought, I think it can keep it like this. but put spaces between different parts of the test.

and change the name of the test. it doesn't check that the daycare is available on specific dates. it checks that a daycare is not available after its capacity been reduced to 0.

You right thanks for making it clear, Fixed.

Omeramsc · 2022-05-11T15:25:56Z

main/tests.py

@@ -76,4 +78,59 @@ def test_dog_owner_homepage_is_visible_for_dog_owner(self, client, create_dog_ow
        client.force_login(user=create_dog_owner_user.user)
        response = client.get("/homepage/")
        assert response.status_code == 200
-        assert list(response.context['daycares']) == list(DayCare.objects.all())
+        assert set(response.context['day_care_queryset']) == set(DayCare.objects.all())


If this test is ensuring that the page will have a list of all the day cares, maybe it should be mentioned in its name. or in a different test.

NP , Fixed by changing the name to " test_dog_owner_homepage_present_all_daycares"

Adding area choices for make the data model compatible, which will improve search for day care by area results. Signed-off-by: tamirmatok <[email protected]>

The dog-owner user can search for day-care that relevant for him. If the user didnt search for day-care he will see all Day-Cares in the application. Signed-off-by: tamirmatok <[email protected]>

Capacity dates check for show only available daycares in dates range. "get_capacity_of_daycare_in_dates_range" function took from redhat-beyond#95 . waiting for approval. Signed-off-by: tamirmatok <[email protected]>

ormergi

Hi Tamir,
Note that there is no need to add . at the end of a commit message title, please remove them.
There is typo at second commit title funconallity instead of functionality.
Please see my inline comments

ormergi · 2022-05-25T06:35:09Z

conftest.py

@@ -76,7 +76,7 @@ def daycare_data():
    pytest.DESCRIPTION = "This is the first daycare test"
    pytest.PRICE_PER_DAY = 10
    pytest.CAPACITY = 50
-    pytest.AREA = "Merkaz"
+    pytest.AREA = "C"


nit: Why not using the full name? 🙂

ormergi · 2022-05-25T06:42:37Z

daycare/forms.py

+
+    area = forms.ChoiceField(required=False, label='Area',
+                             choices=(Area.choices + [('', 'All'), ]), initial="")
+    city = forms.CharField(required=False, label='City')


The UI is an entry point for this app and as such it should perform validation on the input,
regardless if the users are static or not.
Nothing prevents for the next dev to create another static user and enter non valid city name, right?
If this is a lot of work you can do it on a follow up PR and write in the PR description about it.

ormergi · 2022-05-25T06:49:50Z

main/tests.py

+        client.force_login(user=create_dog_owner_user.user)
+        response = client.post('/homepage/', search_form, follow=True)
+        day_care_queryset = response.context['day_care_queryset']
+        assert daycare_user in day_care_queryset


It would make the test look nicer if the assertion would be separated with newline from the test setup logic (all over this file).

ormergi · 2022-05-25T06:57:20Z

main/tests.py

+        expected_queryset = available_day_cares.intersection(filter_day_cares)
+        assert set(day_care_queryset) == set(expected_queryset)
+
+    def test_search_for_day_care_with_start_date_greater_than_end_date_show_error_and_present_all_daycares(


By mentioning with_start_date_greater_than_end_date it makes me think this is the only case all daycares will show.
Would it make sense to rename this test to something like:
test_dog_owner_page_view_presents_all_daycares_when_search_results_is_empty?
In other words, would all daycare show in case of a search error?

Omeramsc · 2022-05-26T14:30:48Z

Conflicts preventing this from merging. Sorry, It probably won't be able to get in for the demo.

tamirmatok added the Status/In-Progress label Apr 27, 2022

tamirmatok self-assigned this Apr 27, 2022

tamirmatok force-pushed the search-daycares branch 2 times, most recently from 320fec9 to 408ddb7 Compare April 30, 2022 13:27

tamirmatok marked this pull request as ready for review April 30, 2022 13:32

tamirmatok added Ready for review and removed Status/In-Progress labels Apr 30, 2022

tamirmatok requested review from Omeramsc, ErezCohenn, ormergi, jeniaSakirko, OfirMatasas and Yuval-Vino April 30, 2022 13:33

tamirmatok added Status/In-Progress and removed Ready for review labels Apr 30, 2022

tamirmatok marked this pull request as draft April 30, 2022 18:08

tamirmatok force-pushed the search-daycares branch from 408ddb7 to 2dcf8d7 Compare April 30, 2022 18:10

Yuval-Vino mentioned this pull request May 1, 2022

Adding message and chat HTML #100

Merged

5 tasks

tamirmatok force-pushed the search-daycares branch 2 times, most recently from b5ebfcd to 8b32e15 Compare May 2, 2022 13:51

tamirmatok marked this pull request as ready for review May 2, 2022 13:57

tamirmatok added the Ready for review label May 2, 2022

tamirmatok force-pushed the search-daycares branch from 8b32e15 to d573c3f Compare May 2, 2022 14:00

tamirmatok removed the Status/In-Progress label May 2, 2022

Yuval-Vino reviewed May 2, 2022

View reviewed changes

tamirmatok force-pushed the search-daycares branch 2 times, most recently from 6d6b01a to 981fa2f Compare May 2, 2022 22:33

tamirmatok added Status/In-Progress and removed Ready for review labels May 2, 2022

tamirmatok marked this pull request as draft May 3, 2022 00:08

tamirmatok force-pushed the search-daycares branch 2 times, most recently from 86f6365 to ba9bee6 Compare May 3, 2022 10:06

tamirmatok marked this pull request as ready for review May 3, 2022 10:09

tamirmatok added Ready for review and removed Status/In-Progress labels May 3, 2022

ErezCohenn reviewed May 4, 2022

View reviewed changes

tamirmatok force-pushed the search-daycares branch 3 times, most recently from 2175321 to 48c3373 Compare May 4, 2022 09:37

ErezCohenn mentioned this pull request May 8, 2022

Create reservation for dog-owner user #107

Draft

7 tasks

ErezCohenn approved these changes May 10, 2022

View reviewed changes

Omeramsc requested changes May 11, 2022

View reviewed changes

Add area choices to day-care model.

de1deb7

Adding area choices for make the data model compatible, which will improve search for day care by area results. Signed-off-by: tamirmatok <[email protected]>

tamirmatok force-pushed the search-daycares branch from 48c3373 to f6d788a Compare May 18, 2022 18:29

Add search day-care funconallity to dog-owner homepage.

97dd0a4

The dog-owner user can search for day-care that relevant for him. If the user didnt search for day-care he will see all Day-Cares in the application. Signed-off-by: tamirmatok <[email protected]>

tamirmatok force-pushed the search-daycares branch from f6d788a to 3a06865 Compare May 18, 2022 18:33

Add "capacity dates check" for dog owner search.

eea8053

Capacity dates check for show only available daycares in dates range. "get_capacity_of_daycare_in_dates_range" function took from redhat-beyond#95 . waiting for approval. Signed-off-by: tamirmatok <[email protected]>

tamirmatok force-pushed the search-daycares branch from 3a06865 to eea8053 Compare May 18, 2022 19:04

ormergi reviewed May 25, 2022

View reviewed changes

Add search day-care functionality to dog-owner homepage. #89

Are you sure you want to change the base?

Add search day-care functionality to dog-owner homepage. #89

Conversation

tamirmatok commented Apr 27, 2022 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Test coverage

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 2, 2022 • edited Loading

Choose a reason for hiding this comment

ErezCohenn commented May 3, 2022

tamirmatok commented May 3, 2022

OfirMatasas commented May 3, 2022

ErezCohenn left a comment

Choose a reason for hiding this comment

There were no daycares found that match the filtering that was selected

tamirmatok commented May 4, 2022

There were no daycares found that match the filtering that was selected

ErezCohenn commented May 6, 2022

There were no daycares found that match the filtering that was selected

tamirmatok commented May 7, 2022 • edited Loading

OfirMatasas commented May 9, 2022

ErezCohenn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamirmatok May 16, 2022 • edited Loading

Choose a reason for hiding this comment

ormergi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omeramsc commented May 26, 2022

tamirmatok commented Apr 27, 2022 •

edited

Loading

tamirmatok May 2, 2022 •

edited

Loading

tamirmatok May 2, 2022 •

edited

Loading

tamirmatok May 2, 2022 •

edited

Loading

tamirmatok commented May 7, 2022 •

edited

Loading

tamirmatok May 16, 2022 •

edited

Loading

tamirmatok May 16, 2022 •

edited

Loading

tamirmatok May 16, 2022 •

edited

Loading

tamirmatok May 16, 2022 •

edited

Loading

tamirmatok May 18, 2022 •

edited

Loading

tamirmatok May 18, 2022 •

edited

Loading

tamirmatok May 16, 2022 •

edited

Loading