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

Fix for show username in member permissions page #660

Closed
wants to merge 1 commit into from
Closed

Fix for show username in member permissions page #660

wants to merge 1 commit into from

Conversation

kavindya89
Copy link
Contributor

Proposed changes in this pull request

The issue #626 suggest that if full name is empty then username should be set in member permission page. This pull request contains fix for it as well as member list page, when full name is empty member column gets blank. So I include to display username if full name is not present

When should this PR be merged

No Precondtions

Risks

No Potential Risks

oliverroick
oliverroick reviewed Sep 11, 2016
View reviewed changes
cadasta/templates/organization/organization_members.html
{{ user.get_full_name }}
{% else %}
{{ user.username }}
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Your PR looks good. Just one little thing:

In lines 43 and 47 the username is displayed again. I think, we should add a condition that the username is only shown, when there is no full name. Consider using the with template tag to avoid hitting the database multiple times.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that my comment doesn't make any sense. I shouldn't do code reviews on a Sunday morning. Please ignore and sorry for the confusion.

@oliverroick
Copy link
Member

This PR looks good and is ready to go. Thanks for helping us!

In the meantime, I have merged two other PRs into master. Could you please update your branch with those two commits. After that we can merge your additions.

@ian-ross
Copy link
Contributor

Thanks for the PR, @kavindya89! As Oliver says, you'll need to rebase over the latest changes to master (you can read about how we use Git here: https://devwiki.corp.cadasta.org/Git%20workflow).

The intent of what you've done is exactly right, but I'd like to see the code organised slightly differently. Instead of having the if-then logic to determine the display name inside the templates, I think it would be better to have a get_display_name method on the User model that does the determination of what to display as the user's "display name". That way, if we ever decide to change how we display these things, we only need to make a change in one place (the User.get_display_name method) instead of needing to track down every place where we have this logic.

This is something it's almost always a good idea to do in Django: put as much logic as you can into your models, and as little logic as possible into views and templates. Models are much easier to test, for one thing, but they also tend to be used by views and templates much more than things working in the opposite direction.

@ian-ross
Copy link
Contributor

@kavindya89 Please see my comment on PR #675, since I notice that all the same applies to you here!

@kavindya89
Copy link
Contributor Author

@ian-ross @oliverroick Sorry I was away from the PC for two days. Will do the needful

@ian-ross
Copy link
Contributor

No problem! There's no rush with these things.

@kavindya89
Copy link
Contributor Author

@ian-ross I have added a method to the model object. Can you verify the changes and let me know If I done it correctly?

@kavindya89
Copy link
Contributor Author

@ian-ross I noticed check failed. Is there anything I need to correct?

@ian-ross
Copy link
Contributor

@kavindya89 Yes. You can follow the "Details" link next to the test failure message to see what's happening on Travis. You can run all the integration tests locally in your VM using the tox command -- this runs exactly the integration tests that are run on Travis. In this case you have a couple of PEP8 layout violations (picked up by the flake8 tool) and you don't have complete test coverage, so you'll need to write a test to deal with that. You can get detailed test coverage information by following the instructions here: https://devwiki.corp.cadasta.org/Running%20tests%20for%20the%20back%20end

@kavindya89
Copy link
Contributor Author

@ian-ross thank you. Yep I did it and fixed it.

@ian-ross
Copy link
Contributor

This is still failing. The first problem is layout: you need to follow PEP8 faithfully -- look at https://www.python.org/dev/peps/pep-0008/#id21 to see what PEP8 says about blank lines. (This might seem really fussy, but having completely uniform code layout makes a big difference when you're looking through the code base.) The second problem is test coverage: the lines you've added to cadasta/account/models.py are not covered by any tests. You'll need to add a test to cover those lines. You can run the tests to get coverage information by following the instructions at https://devwiki.corp.cadasta.org/Running%20tests%20for%20the%20back%20end

In addition, as I mentioned above, for us to be able to merge a PR with these changes, you need to follow the instructions in this comment on issue #675: #675 (comment)

@ian-ross
Copy link
Contributor

If you run the tests with coverage analysis enabled, you'll see that one line (https://github.com/Cadasta/cadasta-platform/pull/660/files#diff-1ec82efacfd767281c32995e3d9b1547R76) isn't covered by a test. You need to figure out a way to make one of the tests go into that branch of that if statement.

@kavindya89
Copy link
Contributor Author

@ian-ross thanks a lot for your inputs. I'll go through them carefully and fix the issues. Lot's of new things learning. Thanks again.

@ian-ross
Copy link
Contributor

@kavindya89 Test coverage is kind of annoying, but it's a thing we do. And Git is complicated. Lots of people have trouble with it. I still get into a tangle with it now and then.

@kavindya89 kavindya89 force-pushed the master branch 2 times, most recently from 139e318 to b27043a Compare September 14, 2016 18:58
@kavindya89
Fix for show username in member permissions page
3179715 
The issue Cadasta#626 suggest that if full name is empty
then username should be set in member permission page. This pull request contains fix for it as well as
member list page, when full name is empty member column gets blank. So I include to display username if full name is not
present
@kavindya89
Copy link
Contributor Author

@ian-ross fixed issues and create a pull request in #687 according to guidelines. Let me know anything to be correct

@ian-ross
Copy link
Contributor

Superseded by #687

@ian-ross ian-ross closed this Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kavindya89 @oliverroick @ian-ross