-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add organizations to user profile #5583
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
One issue is that this area is cached within a higher level fragment cache.
As you can see in that cache key, there is a key called github_repos_updated_at
which indicates when something has changed about a user's GitHub repos.
We should have a similar attribute on users: organization_info_updated_at
which is touched
when a user joins a new org.
If you can create that column and add it to this cache key I think we're good to go!
@benhalpern Oops my bad, I did not notice the higher level fragment cache. Thank you! |
9285367
to
1cbe333
Compare
db/migrate/20200119214529_add_organization_info_updated_at_to_users.rb
Outdated
Show resolved
Hide resolved
a255fe3
to
d7588b3
Compare
@rhymes Fixed the 3 failing tests by updating the
|
Very nice PR @seb1441, thanks for this! 👏 |
@seb1441 I went ahead and resolved the conflict (the schema file wasn't up to date). Keep in mind that it's better to use a branching model than to submit PRs from your own master. You can check the contributing guide on how to do it in the future! |
@rhymes Thank you! Also, I was wondering, when we create a PR, are we expected to keep it up-to-date all the time? |
@seb1441 no, unless you need to explicitly merge something from master or rebase a schema conflict you're not expected to track master during the lifetime of the PR :) Unfortunately the build failed because Right now we've merged #5643 to fix that, so I'm afraid you'll have to rebase to make sure. I'm sorry for the mishap and thanks for the patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once performance suggestion otherwise this is looking great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a note about the schema, we're almost there!
Thanks for the patience @sebach1!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @seb1441!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @seb1441!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Sorry everyone who reviewed, after merging #5624 a conflict was introduced here. I had to resolve it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, excellent first contribution @seb1441 😃 Thanks for fixing the conflict @rhymes.
Requested changes have been implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Nice work everybody involved in the review.
What type of PR is this? (check all applicable)
Description
Added organizations to user profile in the right-hand sidebar.
Related Tickets & Documents
Closes #4672
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?