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

Auth: attach house model to django user #111

Merged
merged 1 commit into from
May 19, 2022
Merged

Auth: attach house model to django user #111

merged 1 commit into from
May 19, 2022

Conversation

ronyyosef
Copy link
Collaborator

@ronyyosef ronyyosef commented May 1, 2022 •
edited
Loading

Relevant issue: #91 #84

  1. Change the House model to be with a one-to-one relationship with the Django user model
  2. fix the create_mock_data to the new data models
  3. Change the login page to be Django user login
  4. Add House after the first Login with the Django user

@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch 5 times, most recently from 290c61e to 480d4ff Compare May 1, 2022 18:33
@ronyyosef
Copy link
Collaborator Author

More Tests will be added soon

@ronyyosef ronyyosef self-assigned this May 2, 2022
ShellyGolden
ShellyGolden reviewed May 2, 2022
View reviewed changes
house/views.py Show resolved Hide resolved
static/css/house_create.css Outdated Show resolved Hide resolved
templates/house_view/house_create.html Outdated Show resolved Hide resolved
@ronyyosef ronyyosef requested a review from ShellyGolden May 2, 2022 22:07
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from c541fe6 to 8385617 Compare May 2, 2022 22:09
@ShellyGolden
Copy link
Collaborator

Can you add screenshots? @ronyyosef
thanks

@ronyyosef
Copy link
Collaborator Author

Login page
image

Sign up page
image

Create house view after first sign up
Uploading image.png…

@ronyyosef
Copy link
Collaborator Author

Added option to give the create_mock_data input of how many users to create, default 500
Mock users format:
username: username 1...500
password: password

@ronyyosef ronyyosef requested a review from ShellyGolden May 4, 2022 09:56
@LiorKGOW LiorKGOW mentioned this pull request May 4, 2022
Closed
@LiorKGOW
Copy link
Collaborator

LiorKGOW commented May 4, 2022

Could you attach the screenshots to the first comment of the PR?
So it would be more visible

LiorKGOW
LiorKGOW reviewed May 4, 2022
View reviewed changes
Copy link
Collaborator

@LiorKGOW LiorKGOW left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey Rony
Nice work on the PR
I have left you some comments

Don't you think this PR is a bit too big? Do you think it could be devided to several PRs?
Also there is something wrong with the commits, I think it's the new lines missing at the end of some files,
I tried to find them all, and marked them 👍

scripts/tests.py Show resolved Hide resolved
house/models.py Show resolved Hide resolved
house/models.py Outdated Show resolved Hide resolved
house/models.py Show resolved Hide resolved
house/models.py Outdated Show resolved Hide resolved
house/migrations/0001_initial.py Outdated Show resolved Hide resolved
factories/user.py Outdated Show resolved Hide resolved
expenses/test/test_expenses.py Outdated Show resolved Hide resolved
@ronyyosef
Copy link
Collaborator Author

Hey Rony Nice work on the PR I have left you some comments

Don't you think this PR is a bit too big? Do you think it could be devided to several PRs? Also there is something wrong with the commits, I think it's the new lines missing at the end of some files, I tried to find them all, and marked them 👍

Yee, this PR is a big big, A lot of things depend on house so I have to fix all after I change the data model
thanks, I will go trow all of them tomorrow

@ShellyGolden ShellyGolden mentioned this pull request May 4, 2022
Merged
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from 8385617 to 66eb61c Compare May 5, 2022 08:17
@ronyyosef ronyyosef requested a review from LiorKGOW May 5, 2022 08:18
shlior7
shlior7 reviewed May 8, 2022
View reviewed changes
house/forms.py Outdated Show resolved Hide resolved
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from 3522092 to d0be8ca Compare May 16, 2022 15:52
@ShellyGolden ShellyGolden mentioned this pull request May 17, 2022
Merged
svatares
svatares reviewed May 17, 2022
View reviewed changes
Copy link
Contributor

@svatares svatares left a comment

Choose a reason for hiding this comment

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

Once the last opened conversation is resolved this MR can be merged

@ronyyosef
Copy link
Collaborator Author

@svatares ready for merge

Yoavsc129
Yoavsc129 suggested changes May 18, 2022
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch 4 times, most recently from 955675b to ed05168 Compare May 19, 2022 07:35
@ronyyosef ronyyosef requested a review from svatares May 19, 2022 07:36
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from ed05168 to 15d6e1b Compare May 19, 2022 07:40
LiorKGOW
LiorKGOW suggested changes May 19, 2022
View reviewed changes
Copy link
Collaborator

@LiorKGOW LiorKGOW left a comment

Choose a reason for hiding this comment

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

Hey Rony, I had a question about your PR, and a suggestion to change the title of the house creation page

templates/house_view/house_create.html Outdated Show resolved Hide resolved
tests/house_tests.py Show resolved Hide resolved
@LiorKGOW LiorKGOW added question Further information is requested and removed Ready to merge labels May 19, 2022
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from 15d6e1b to 3b8bc7d Compare May 19, 2022 08:09
@svatares
Copy link
Contributor

@ronyyosef I also tried and it works fine.
There is one issue though, when singing-up, the public checkbox is required, while it should be optional.
Meaning, right now, a user can't create a house which is private.
Please fix that, and add a test that checks both cases, that a public/private houses can be generated

LiorKGOW
LiorKGOW approved these changes May 19, 2022
View reviewed changes
Copy link
Collaborator

@LiorKGOW LiorKGOW left a comment

Choose a reason for hiding this comment

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

Thank you for the quick response @ronyyosef 💪

LiorKGOW
LiorKGOW reviewed May 19, 2022
View reviewed changes
house/forms.py Outdated Show resolved Hide resolved
@LiorKGOW LiorKGOW removed the question Further information is requested label May 19, 2022
@ronyyosef
Copy link
Collaborator Author

ronyyosef commented May 19, 2022 •
edited
Loading

Thank you for the quick response @ronyyosef 💪

Thank you for noticing, It was a bug
Fixed now

here are the tests that check the fix:
image
test_public_house, test_private_house

@ronyyosef
Add House creation logic after login as a user
0fb2aee 
Add Create House View
fixing House tests and model to new house model
fixing expenses tests and model to new house model
Fixing create mock data
Adding test for script
Adding test for factories
Change the House model to be with a one-to-one relationship with the Django user model
@ronyyosef ronyyosef force-pushed the attach-user-auth-house-model branch from 3b8bc7d to 0fb2aee Compare May 19, 2022 09:13
@svatares svatares merged commit 4ecb112 into redhat-beyond:main May 19, 2022
@ronyyosef ronyyosef deleted the attach-user-auth-house-model branch May 19, 2022 21:13
shlior7 pushed a commit to shlior7/Xpense that referenced this pull request May 23, 2022
@svatares @shlior7
Merge pull request redhat-beyond#111 from ronyyosef/attach-user-auth-…
cecc6e4 
…house-model

Auth: attach house model to django user
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiorKGOW LiorKGOW LiorKGOW approved these changes

@ShellyGolden ShellyGolden ShellyGolden approved these changes

@svatares svatares svatares approved these changes

@Yoavsc129 Yoavsc129 Yoavsc129 approved these changes

@adiabramovitch adiabramovitch Awaiting requested review from adiabramovitch

@orIsrael orIsrael Awaiting requested review from orIsrael

@shlior7 shlior7 Awaiting requested review from shlior7

Assignees

@adiabramovitch adiabramovitch

@ronyyosef ronyyosef

Labels
Ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronyyosef @ShellyGolden @LiorKGOW @adiabramovitch @svatares @shlior7 @Yoavsc129