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

#364 Added Models for Users Profile. #469

Merged
merged 32 commits into from
Sep 20, 2016

Conversation

RishabhJain2018
Copy link
Contributor

No description provided.

city=models.CharField(max_length=100, blank=False, null=False)
contact_no = models.CharField(max_length=10, blank=False, null=False)

created = models.DateTimeField("Created", null=True, auto_now_add=True)
Copy link
Member

Choose a reason for hiding this comment

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

Look in base/models, there is already mix-in for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not get you properly ?? Please explain to me.

Copy link
Member

Choose a reason for hiding this comment

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

Don't add created/modified, there is already mixins for this. Have a look at junction/base/models.py. AuditModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I got that. Thanks for guiding. :)

@kracekumar
Copy link
Member

  • I can't find how user profile will be created, can you add that logic as well ?
  • You need to write test case.

@RishabhJain2018
Copy link
Contributor Author

I am on writing tests. I will send a PR soon.

@RishabhJain2018
Copy link
Contributor Author

I have written tests for django but I am getting one error . I am uploading it please see to it and guide me.
tests_error

@kracekumar
Copy link
Member

pip install -r requirements-dev.txt https://github.com/pythonindia/junction/blob/master/requirements-dev.txt

@RishabhJain2018
Copy link
Contributor Author

@kracekumar I tried the command you mentioned above, but it is giving me some unexpected error. I also googled it but could not find anything relevant. Please look into it as I am putting the screenshot.
error2

@ChillarAnand
Copy link
Member

Have you modified requirements.txt or requirements-dev.txt? Make sure there are no syntax errors in those files.

@RishabhJain2018
Copy link
Contributor Author

@ChillarAnand No, I haven't modified any of these files. Well I am running the junction module without docker. Is it because of that ?
please check these files , it gives error when a new user is installing. I have kept the screenshot of error above.

@kracekumar
Copy link
Member

@RishabhJain2018 Can you paste the requirements file and can you merge master to your branch ?

@RishabhJain2018
Copy link
Contributor Author

@kracekumar yeah sure.

@RishabhJain2018
Copy link
Contributor Author

RishabhJain2018 commented Jul 8, 2016

@kracekumar this is the requirements file & I have merged the master branch.
requirements

@RishabhJain2018
Copy link
Contributor Author

RishabhJain2018 commented Jul 8, 2016

@kracekumar Please help.

@kracekumar
Copy link
Member

Ok, I think you're using older version of pip. Upgrade the pip pip install -U pip and run the pip install -r requirements-dev.txt.

@RishabhJain2018
Copy link
Contributor Author

@kracekumar thanks for solving the issue. I upgraded my pip & it worked. I am again facing an error due to which my build is failing. please see to it.
tests

@kracekumar
Copy link
Member

@RishabhJain2018 Please attach screen shot only for UI changes, else pass on the link or paste the output. Exception says no such table: profiles_profile. You need to add migration file. Read here.

@coveralls
Copy link

coveralls commented Jul 9, 2016

Coverage Status

Coverage increased (+0.07%) to 72.078% when pulling 0d2ff38 on RishabhJain2018:master into 6451feb on pythonindia:master.

@RishabhJain2018
Copy link
Contributor Author

@kracekumar Thanks for the guidance. :)



class UserAdmin(admin.ModelAdmin):
list_display = ('__unicode__', 'city', 'contact_no')
Copy link
Member

@kracekumar kracekumar Jul 9, 2016

Choose a reason for hiding this comment

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

Why is this __unicode__ required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to display user name in Profile admin. corresponding to whom the city and contact_no belongs.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove __unicode__.

@kracekumar
Copy link
Member

Before pushing the code, please rebase with the master branch.

@coveralls
Copy link

coveralls commented Sep 4, 2016

Coverage Status

Coverage decreased (-0.1%) to 68.966% when pulling 6eede3a on RishabhJain2018:master into 9d244ed on pythonindia:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage decreased (-0.2%) to 68.839% when pulling 72e8f75 on RishabhJain2018:master into faf9607 on pythonindia:master.

@RishabhJain2018
Copy link
Contributor Author

RishabhJain2018 commented Sep 9, 2016

@kracekumar @theskumar I have made the changes as per the requirement. Please review it.


elif request.method == "GET":
user = get_object_or_404(User, pk=username.id)
detail = get_object_or_404(Profile, user=user)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we create a profile before this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChillarAnand I didn't get you. Please explain in detail.

Copy link
Member

Choose a reason for hiding this comment

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

When a user visits edit page, it is simply throwing a 404 page. Shouldnt we create a profile before if it doesnt exists for that user?

Copy link
Contributor Author

@RishabhJain2018 RishabhJain2018 Sep 17, 2016

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChillarAnand I have fixed the changes . Please see to it.

@RishabhJain2018
Copy link
Contributor Author

RishabhJain2018 commented Sep 15, 2016

@ChillarAnand @kracekumar Please review the PR & tell me any other changes that needs to be done. 😃

@coveralls
Copy link

coveralls commented Sep 15, 2016

Coverage Status

Coverage decreased (-0.2%) to 68.839% when pulling 5bf8175 on RishabhJain2018:master into faf9607 on pythonindia:master.

@coveralls
Copy link

coveralls commented Sep 17, 2016

Coverage Status

Coverage decreased (-0.2%) to 68.839% when pulling 8db1f43 on RishabhJain2018:master into faf9607 on pythonindia:master.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage decreased (-0.6%) to 68.479% when pulling caafce3 on RishabhJain2018:master into faf9607 on pythonindia:master.



class ProfileAdmin(admin.ModelAdmin):
list_display = ('__unicode__', 'city', 'contact_no')
Copy link
Member

Choose a reason for hiding this comment

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

Can you use user instead of unicode?

Copy link
Contributor Author

@RishabhJain2018 RishabhJain2018 Sep 18, 2016

Choose a reason for hiding this comment

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

yeah sure. but can you explain me why user is better?

@ChillarAnand
Copy link
Member

@RishabhJain2018 Looks good. There are several migrations. Remove all files from migrations folder except __init__.py and run makemigrations & migrate, it should squash them to one.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage decreased (-0.6%) to 68.479% when pulling 7685efc on RishabhJain2018:master into faf9607 on pythonindia:master.

@RishabhJain2018
Copy link
Contributor Author

@ChillarAnand I have done the changes and also squashed the migrations.

@ChillarAnand ChillarAnand merged commit d92a645 into pythonindia:master Sep 20, 2016
@ChillarAnand
Copy link
Member

Excellent. Thank you @RishabhJain2018

@RishabhJain2018 RishabhJain2018 mentioned this pull request Feb 11, 2017

@login_required
def profile(request):
username = request.user
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename the variable to user?

username = request.user
detail = None

if request.method == "POST" and username == request.user:
Copy link
Member

Choose a reason for hiding this comment

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

Can I know why is the check username == request.user required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants