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

Add home page and URLs redirecting #70

Merged
merged 4 commits into from
Dec 22, 2021
Merged

Add home page and URLs redirecting #70

merged 4 commits into from
Dec 22, 2021

Conversation

shayakrach
Copy link
Collaborator

@shayakrach shayakrach commented Dec 17, 2021 •
edited
Loading

Moving all job URLs to a new file to make code maintainable
and readable.
Adding home page and set as the default page since jobs page is for logged-in users.
Adding URL redirecting.

close #62, #67.

To make this PR ready I have added 2 issues that follow it #71, #72 that will be fixed after demo 3.

@shayakrach shayakrach force-pushed the add-tests branch 7 times, most recently from 923f8cd to 4a6041c Compare December 17, 2021 15:16
@shayakrach shayakrach changed the title Add views tests WIP Add views tests Dec 17, 2021
@assafg95
Copy link
Collaborator

Looks great!

AshkenYariv
AshkenYariv approved these changes Dec 17, 2021
View reviewed changes
Copy link
Collaborator

@AshkenYariv AshkenYariv left a comment

Choose a reason for hiding this comment

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

@shayakrach looks good, both the homepage and the tests!

@ormergi
Copy link
Contributor

ormergi commented Dec 18, 2021

Hi Shay, please change the PR title to reflect what this PR does (most part is adding a home page)
and the change its description to reflect what changes its brings and why you need them.
Note about tests, they should be integral part of your PR, there is no need to mention it on the description or on commit body messages (other than the title).

@shayakrach shayakrach changed the title Add views tests Add home page and refactor job views Dec 18, 2021
ormergi
ormergi requested changes Dec 18, 2021
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.

First commit,
The title is too general, this commit is about moving stuff to files so better title would be "Extract jobs URL's to file".
As for the commit body it should describe why you need this change for the most part, the what should be understood by the implementation.

@shayakrach shayakrach force-pushed the add-tests branch 2 times, most recently from 62429db to 17d4954 Compare December 18, 2021 18:35
ormergi
ormergi reviewed Dec 18, 2021
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.

second commit
Regarding the commit message body, there is no need to repeat the title (Add home page == creating home page)

templates/home.html Outdated Show resolved Hide resolved
@shayakrach shayakrach force-pushed the add-tests branch 2 times, most recently from 05a68d2 to e248e95 Compare December 18, 2021 18:41
ormergi
ormergi requested changes Dec 18, 2021
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.

3rd commit:
Correct me if I am wrong, but this commit is about changing the app url redirection behavior following the new home page that has been added, right?
If so this is not refactoring, w/o these changes the home page wont show and its a dead code,
thus this commit should be part of the commit that adds the home page.

@shayakrach
Copy link
Collaborator Author

Hi Shay, please change the PR title to reflect what this PR does (most part is adding a home page) and the change its description to reflect what changes its brings and why you need them. Note about tests, they should be integral part of your PR, there is no need to mention it on the description or on commit body messages (other than the title).

Fixed @ormergi .

ormergi
ormergi requested changes Dec 18, 2021
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.

4th commit:
Consider moving the files renaming and moving tests around on a separate PR it is not related to adding a home pare right? 🙂

As for the home page tests, you can also move them to the commit that adds the new business logic, not mandatory for now..

@ormergi
Copy link
Contributor

ormergi commented Dec 18, 2021 •
edited
Loading

Fixed @ormergi .

I think you better go though all my comments first 🙂

@shayakrach
Copy link
Collaborator Author

second commit Regarding the commit message body, there is no need to repeat the title (Add home page == creating home page)

Fixed @ormergi.

@shayakrach
Copy link
Collaborator Author

Fixed @ormergi .

I think you better go though all my comments first 🙂

Yes, I read all comments.
Fixing now all the problems.

@shayakrach
Copy link
Collaborator Author

he title is too general, this commit is about moving stuff to files so better title would be "Extract jobs URL's to file".
As for the commit body it should des

Fixed.

@shayakrach
Copy link
Collaborator Author

Correct me if I am wrong, but this commit is about changing the app url redirection behavior following the new home page that has been added, right?
If so this is not refactoring, w/o th

I changed to a better title which explains why it should be included in this PR.

@shayakrach
Copy link
Collaborator Author

4th commit: Consider moving the files renaming and moving tests around on a separate PR it is not related to adding a home pare right? 🙂

As for the home page tests, you can also move them to the commit that adds the new business logic, not mandatory for now..

But I have to test all the redirecting logic in the same PR, no?

ormergi
ormergi reviewed Dec 19, 2021
View reviewed changes
jobs/views.py Outdated Show resolved Hide resolved
@ormergi
Copy link
Contributor

ormergi commented Dec 19, 2021 •
edited
Loading

But I have to test all the redirecting logic in the same PR, no?

Yes in general you need to test the logic this PR brings, here it adds home page and set to be the default.
If this was the PR description it would be much more easy to understand what this PR does.
Especially the 4th commit message, its confusing I didn't realized at first that all tests are new and necessary.

Next time keep all the related changes on the same commit, for example on this PR:
you could merge 3&4 commits, then you would have a commit that adds a new page and another one that changes the behavior of the app accordingly + tests for the new logic you added.

For the sake of progressing and merge this PR, I wont insist on merging commits for now
but for following PR's consider what we discussed here 🙂

@ormergi
Copy link
Contributor

ormergi commented Dec 19, 2021

Regarding the second commit message, its about setting the new home page as default, right?
If so, the commit title should express exactly that. And the commit body should explain why you need the change, and how its going to affect other components (like you did which is great).

Same goes for other commits on this PR, please revisit them and think how you can improve.

@shayakrach shayakrach force-pushed the add-tests branch 2 times, most recently from 0ae9025 to 944b322 Compare December 19, 2021 21:28
@shayakrach
Copy link
Collaborator Author

shayakrach commented Dec 19, 2021 •
edited
Loading

But I have to test all the redirecting logic in the same PR, no?

Yes in general you need to test the logic this PR brings, here it adds home page and set to be the default. If this was the PR description it would be much more easy to understand what this PR does. Especially the 4th commit message, its confusing I didn't realized at first that all tests are new and necessary.

Next time keep all the related changes on the same commit, for example on this PR: you could merge 3&4 commits, then you would have a commit that adds a new page and another one that changes the behavior of the app accordingly + tests for the new logic you added.

For the sake of progressing and merge this PR, I wont insist on merging commits for now but for following PR's consider what we discussed here 🙂

Yes, I understand it now.
I will be more clear and accurate in the next PR.
So is that okay for now @ormergi?

@assafg95
Copy link
Collaborator

Everything looks good!

AshkenYariv
AshkenYariv approved these changes Dec 20, 2021
View reviewed changes
Copy link
Collaborator

@AshkenYariv AshkenYariv left a comment

Choose a reason for hiding this comment

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

Looks good!

ormergi
ormergi requested changes Dec 22, 2021
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.

First commit message: typo "Add" -> added

Second commit message, this part is confusing "..instead of the jobs page since
this page should only be visible to logged-in users".
Which page is "this page"? please be explicit with the page you refer to (e.g: home page, job page..) and change it accordingly.

Last commit: Remove this line "Refactoring jobs test."

Maybe I wasn't clear about the action items (AI), change the PR description and title to reflect what this PR does (Adding a home pare and set it as default since jobs page is for logged it users, no refactoring..).

@shayakrach
Move jobs URLs to a new file
91f954b 
Moving all job URLs into jobs/urls.py to make code maintainable
and readable. From now on, any URL related to the jobs model will be
added to jobs/urls.py only.
@shayakrach
Add home page and set as default page
c4458da 
Setting home page as the default page instead of the jobs page since
the jobs page should only be visible to logged-in users.
@shayakrach
Add and change urls redirecting
dc89e65 
Redirect unauthorized users to home when accessing jobs pages.
Redirecting student users to the jobs page when accessing HR pages.
Redirecting authorized to the home page after logout.
@shayakrach
Add job views tests
edffcd0 
Renaming the test.py file and placing it in the jobs/tests folder
for a more organized app.
Adding jobs views tests.
@shayakrach shayakrach changed the title Add home page and refactor job views Add home page and URLs redirecting Dec 22, 2021
@shayakrach
Copy link
Collaborator Author

First commit message: typo "Add" -> added

Second commit message, this part is confusing "..instead of the jobs page since this page should only be visible to logged-in users". Which page is "this page"? please be explicit with the page you refer to (e.g: home page, job page..) and change it accordingly.

Last commit: Remove this line "Refactoring jobs test."

Maybe I wasn't clear about the action items (AI), change the PR description and title to reflect what this PR does (Adding a home pare and set it as default since jobs page is for logged it users, no refactoring..).

Fixed.

@ormergi
Copy link
Contributor

ormergi commented Dec 22, 2021

Thanks!

@ormergi ormergi merged commit 3de8b53 into redhat-beyond:master Dec 22, 2021
@AshkenYariv AshkenYariv mentioned this pull request Dec 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ormergi ormergi ormergi requested changes

@assafg95 assafg95 assafg95 approved these changes

@AshkenYariv AshkenYariv AshkenYariv approved these changes

@edenmaor edenmaor Awaiting requested review from edenmaor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Home Page
5 participants
@shayakrach @assafg95 @ormergi @edenmaor @AshkenYariv