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.

Sign up for GitHub

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

Jump to bottom

Adding user's info to the navbar #77

Merged
merged 1 commit into from
May 26, 2022
Merged

Adding user's info to the navbar #77

merged 1 commit into from
May 26, 2022

Conversation

OfirMatasas
Copy link
Collaborator

@OfirMatasas OfirMatasas commented Apr 17, 2022 •
edited
Loading

Desired Outcome

Every user on Hotails will have his profile picture and their nickname on the navbar, following to every page on Hotails.

Implemented Changes

The base_template has changed, that only when a user is logged into Hotails, they'll see this information.

Connected Issue/Story

Closes #76

Definition of Done

  • Includes the profile picture of the user
  • Includes the nickname of the user
  • Appears on every page on Hotails

Test Coverage

This PR includes new unit and integration tests to go with the code

A Picture of The New Navbar

Untitled

@OfirMatasas OfirMatasas self-assigned this Apr 17, 2022
@tamirmatok tamirmatok self-assigned this Apr 18, 2022
ErezCohenn
ErezCohenn reviewed Apr 19, 2022
View reviewed changes
Copy link
Collaborator

@ErezCohenn ErezCohenn left a comment

Choose a reason for hiding this comment

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

Nice job!! looks professional 😄
just take a note of the comment I left

main/views.py Outdated Show resolved Hide resolved
@tamirmatok tamirmatok force-pushed the profile-sidebar branch 6 times, most recently from dc00d15 to 708328e Compare April 20, 2022 10:51
@tamirmatok
Copy link
Collaborator

tamirmatok commented Apr 20, 2022 •
edited
Loading

Day_care_validation
extend navbar

Hi @OfirMatasas , please update PR description with relevant changes (:
In the pictures you can see the navbar for day-care and dog-owner users .

@ormergi
Copy link
Contributor

ormergi commented Apr 21, 2022

ping

@OfirMatasas OfirMatasas marked this pull request as ready for review April 21, 2022 11:05
@OfirMatasas OfirMatasas force-pushed the profile-sidebar branch 3 times, most recently from 650c0ea to 643d7d8 Compare April 21, 2022 11:15
@OfirMatasas OfirMatasas changed the title Profile sidebar Adding user's info to the navbar Apr 21, 2022
@OfirMatasas OfirMatasas force-pushed the profile-sidebar branch 3 times, most recently from ea77a1e to d55e838 Compare April 21, 2022 11:34
@Omeramsc Omeramsc added the invalid This doesn't seem right label May 1, 2022
@jeniaSakirko
Copy link
Contributor

Some parts of this PR are identical to #67 , but there is only 1 commit. please fix it by separating the commits of the old and new content.

@OfirMatasas ^^

@jeniaSakirko jeniaSakirko reopened this May 3, 2022
@OfirMatasas
Copy link
Collaborator Author

Some parts of this PR are identical to #67 , but there is only 1 commit. please fix it by separating the commits of the old and new content.

@OfirMatasas ^^

After a talk with Omer, the plan is to merge the dog owner homepage before this PR, and then after rebasing the common code will be part of the master, and it will be ok.

@ErezCohenn
Copy link
Collaborator

Some parts of this PR are identical to #67 , but there is only 1 commit. please fix it by separating the commits of the old and new content.

@OfirMatasas ^^

After a talk with Omer, the plan is to merge the dog owner homepage before this PR, and then after rebasing the common code will be part of the master, and it will be ok.

so don't forget to add to the description of the PR that this PR depends on #67 :)

@OfirMatasas OfirMatasas force-pushed the profile-sidebar branch 2 times, most recently from d4113d2 to 05ce7f4 Compare May 6, 2022 12:06
@OfirMatasas OfirMatasas added Ready for review and removed Status/In-Progress invalid This doesn't seem right labels May 6, 2022
@OfirMatasas OfirMatasas force-pushed the profile-sidebar branch 2 times, most recently from a62e093 to d900c53 Compare May 6, 2022 12:09
Yuval-Vino
Yuval-Vino reviewed May 6, 2022
View reviewed changes
dogowner/test_dog_owner.py Outdated Show resolved Hide resolved
main/tests.py Outdated Show resolved Hide resolved
main/tests.py Outdated Show resolved Hide resolved
main/context_processors.py Outdated Show resolved Hide resolved
@OfirMatasas OfirMatasas requested a review from Yuval-Vino May 9, 2022 20:15
Omeramsc
Omeramsc reviewed May 11, 2022
View reviewed changes
daycare/test_image.py Outdated Show resolved Hide resolved
Omeramsc
Omeramsc reviewed May 11, 2022
View reviewed changes
daycare/test_image.py Outdated Show resolved Hide resolved
Omeramsc
Omeramsc reviewed May 11, 2022
View reviewed changes
daycare/test_image.py Outdated Show resolved Hide resolved
Omeramsc
Omeramsc reviewed May 11, 2022
View reviewed changes
dogowner/models.py Outdated Show resolved Hide resolved
Omeramsc
Omeramsc reviewed May 11, 2022
View reviewed changes
dogowner/test_dog_owner.py Outdated
dogowner_profile_image = create_dog_owner_user.get_dog_owner_profile_image_url()
dog_owner_default_picture_url = "../../static/images/dog-owner-default-profile-picture.jpeg"
assert dogowner_profile_image != dog_owner_default_picture_url
assert dogowner_profile_image is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

same as I commented about daycare

Omeramsc
Omeramsc requested changes May 11, 2022
View reviewed changes
Copy link
Contributor

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

Left some comments

@OfirMatasas OfirMatasas requested a review from Omeramsc May 16, 2022 21:00
@tamirmatok @OfirMatasas
Adding user picture and username to navbar.
b574897 
When the user is logged into Hotails, they'll see their profile picture,
and their username in every page on Hotails on the navbar.
If the dog owner / daycare doesn't have any profile picture, a default
one will be presented.

Signed-off-by: Ofir Matasas <[email protected]>
Signed-off-by: tamirmatok <[email protected]>
ormergi
ormergi reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@ormergi ormergi left a comment

Choose a reason for hiding this comment

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

Hi, this is nice feature!

There are some formatting and refactoring changes that makes it harder to review this PR,
please separate them to different commits /PRs before the commit that introduce the new logic.

daycare/test_image.py
Comment on lines +27 to +28
assert daycare_profile_image != DAYCARE_DEFAULT_PICTURE_URL
assert daycare_profile_image == DAYCARE_FIXTURE_PROFILE_PICTURE_URL_1
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 its enough to assert on DAYCARE_FIXTURE_PROFILE_PICTURE_URL_1

dogowner/models.py
Comment on lines +61 to +63

def get_dog_owner_profile_image_url(self):
return self.dog_picture_url if self.dog_picture_url else DOG_OWNER_DEFAULT_PROFILE_PICTURE_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isnt this logic part of create()?

main/tests.py
Comment on lines +79 to +81
list_of_daycares_from_response = list(response.context['daycares'])
list_of_all_daycares = list(DayCare.objects.all())
assert list_of_daycares_from_response == list_of_all_daycares
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not related to the scope of this PR, right?

main/tests.py
Comment on lines +89 to +90
dog_owner_picture_url = create_dog_owner_user.dog_picture_url
assert response.context['navbar_picture_url'] == dog_owner_picture_url
Copy link
Contributor

Choose a reason for hiding this comment

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

I would pass variables substitutions in cases like this, and assert the actual value directly instead:

assert response.context['navbar_picture_url'] ==  create_dog_owner_user.dog_picture_url

Also, it would make the test look nicer if the assert line would be separated with new line from the test setup logic.

main/context_processors.py
from dogowner.models import DogOwner


def navbar_extras(request):
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 filler out the authenticated user from the models?
Is it possible to use the logged in user in this context?

conftest.py
@@ -8,6 +8,11 @@
from review.models import Review


DOG_OWNER_FIXTURE_PROFILE_PICTURE_URL = "https://www.akc.org/wp-content/uploads/2019/06/Bohemian-Shepherd.1.jpg"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this change (and similar changes) to their own commit.

Omeramsc
Omeramsc approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

This PR has room to improve, and Or's comments should've been addressed.
however it is good enough, and I won't want this work to not be shown at the last demo, therefore I am merging it.

If you will continue to work on this project later, please address the comments.

@Omeramsc Omeramsc merged commit 3a18d8a into redhat-beyond:main May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ormergi ormergi ormergi left review comments

@tamirmatok tamirmatok tamirmatok approved these changes

@ErezCohenn ErezCohenn ErezCohenn approved these changes

@Omeramsc Omeramsc Omeramsc approved these changes

@jeniaSakirko jeniaSakirko jeniaSakirko approved these changes

@Yuval-Vino Yuval-Vino Awaiting requested review from Yuval-Vino

Assignees

@OfirMatasas OfirMatasas

@tamirmatok tamirmatok

Labels
Ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a sidebar to the base_template
7 participants
@OfirMatasas @tamirmatok @ormergi @Omeramsc @jeniaSakirko @ErezCohenn @Yuval-Vino