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

Show user details. #416

Merged
merged 14 commits into from
Sep 28, 2016
Merged

Conversation

wlonk
Copy link
Contributor

@wlonk wlonk commented Aug 28, 2016

The intent is to eventually be able to go to a user's profile page and see a list of their crates.

See #409.

Every pull request comes with a cute animal:

baby nyala

From http://www.zooborns.com/zooborns/2015/10/houston-zoo-cares-for-valuable-new-gem-opal.html

@wlonk
Copy link
Contributor Author

wlonk commented Aug 28, 2016

I broke ground on this, but it's late at night and I should stop before I write anti-code. I'll mark when this is in a reviewable state!

@wlonk wlonk changed the title [WIP] Show user details. Show user details. Aug 28, 2016
@wlonk
Copy link
Contributor Author

wlonk commented Aug 28, 2016

OK, there's some actual code here if anyone wants to take a look at it!

@alexcrichton
Copy link
Member

Thanks! Do you have some screenshots as well to take a look?

r? @steveklabnik
cc @carols10cents

@wlonk
Copy link
Contributor Author

wlonk commented Aug 30, 2016

I don't at the moment; my local install data has no packages, and because this involved a backend change, I couldn't use the staging data.

@carols10cents
Copy link
Member

OOOOHHH. Let me see if I can get you a screenshot, I have some hacked together data locally I think :) So excited for this!

@carols10cents
Copy link
Member

carols10cents commented Aug 30, 2016

What URLs am I supposed to be able to visit with this code? I'm having trouble telling :-/

It looks like if there is a user in the database with gh_login "carols10cents", that I should be able to go to http://localhost:4200/users/carols10cents/crates, right?

The first problem is that if I'm not logged in, I get told I must be logged in to proceed. But a page of a person or teams' crates should be public, just like a crate's page is, right?

If I log in and go to http://localhost:4200/users/carols10cents/crates, it says "Displaying 1-10 of 5440 total results", which makes me think it's trying to show all the crates, not just the ones this user owns or has authored.

It looks like when I go to http://localhost:4200/users/carols10cents/crates, there's an ajax request going to http://localhost:4200/api/v1/crates?page=1&per_page=10&sort=alpha&user_id=carols10cents&_=1472581741552, but the user_id query in krate.rs is expecting an i32 (https://github.com/rust-lang/crates.io/blob/master/src/krate.rs#L509) and is looking up crates by crate_owners.owner_id... which is the crates.io db id in the users table.

If instead I try to use the users.id number in the URL, like http://localhost:4200/users/3/crates, then I get an error on the page that says Attempted to handle event notFound on <cargo@model:user::ember649:3> while in state root.loaded.updated.uncommitted. and an error in my browser developer tools that says http://localhost:4200/api/v1/users/3?_=1472581995663 is not found.

Here are some relevant entries in my database:

select * from users;
 id | email | gh_access_token | api_token |   gh_login    |            name             |                     gh_avatar                      | gh_id
----+-------+-----------------+-----------+---------------+-----------------------------+----------------------------------------------------+--------
  3 |       | (elided)        | (elided)  | carols10cents | Carol (Nichols || Goulding) | https://avatars.githubusercontent.com/u/193874?v=3 | 193874

select * from crates where id = 5438;
  id  |  name  |         updated_at         |         created_at         | downloads | max_version | description | homepage | documentation | readme | textsearchable_index_col | license | repository | max_upload_size
------+--------+----------------------------+----------------------------+-----------+-------------+-------------+----------+---------------+--------+--------------------------+---------+------------+-----------------
 5438 | zopfli | 2016-08-30 17:42:26.468535 | 2016-08-30 17:42:26.445151 |         0 | 0.3.0       |             |          |               |        | 'zopfli':1A              |         |            |

select * from versions where crate_id = 5438;
  id   | crate_id |  num  |         updated_at         |         created_at         | downloads | features | yanked
-------+----------+-------+----------------------------+----------------------------+-----------+----------+--------
 29606 |     5438 | 0.3.0 | 2016-08-30 17:42:26.461798 | 2016-08-30 17:42:26.461798 |         0 | {}       | f

select * from version_authors where version_id = 29606;
 id | version_id | user_id |     name
----+------------+---------+---------------
  1 |      29606 |       3 | carols10cents

select * from crate_owners where crate_id = 5438;
 id | crate_id | owner_id |         created_at         | created_by | deleted |         updated_at         | owner_kind
----+----------+----------+----------------------------+------------+---------+----------------------------+------------
  4 |     5438 |        3 | 2016-08-30 14:02:58.875959 |          3 | f       | 2016-08-30 14:02:58.875959 |          0

@wlonk
Copy link
Contributor Author

wlonk commented Aug 30, 2016

Yeah, the intended paths are /users/:username and /users/:username/crates.

The auth-required was an oversight, sorry!

The search-by-username not user-id was also an oversight, but of a different sort; I couldn't see that that wasn't working without test data, but I've fixed it now.

I'd love it if you could take another look.

How have you set up local data for testing this? I'd love to not be shooting in the dark!

@carols10cents
Copy link
Member

How have you set up local data for testing this? I'd love to not be shooting in the dark!

It's... pretty ad-hoc. I have a script that imports half the information from the crates index, but then I decided that was a bad idea, but that got me the crates and their versions. I just inserted a few records straight into my database with psql for the owners/authors just now :-/

@carols10cents
Copy link
Member

Taking another look-- I can indeed visit http://localhost:4200/users/carols10cents/crates now without being logged in, yay! And it's saying "Displaying 1 of 1 total results" instead of all of them, yay!

Only problem is... that's all it says, I don't actually see any crates. I do see the crate I expect to see in the json response for http://localhost:4200/api/v1/crates?page=1&per_page=10&sort=alpha&user_id=3&_=1472592537012, so something might not be hooked up right?

And when I try to go to http://localhost:4200/users/carols10cents/, I get an error that says "Assertion Failed: You cannot pass undefined as id to the store's find method" at "EmberError@http://localhost:4200/assets/vendor.js:26338:15". I don't see any errors from the ajax response on that page.

Let me create a file of some data that you should be able to import into your database, 1 min :)

@wlonk
Copy link
Contributor Author

wlonk commented Aug 30, 2016

Thanks for all this feedback! I'll get some sample data so I'm not requiring you to test for me, and get this working. Adding [WIP] tag again.

@wlonk wlonk changed the title Show user details. [WIP] Show user details. Aug 30, 2016
@carols10cents
Copy link
Member

Here's a bit of the data I was using-- after migrating your database, you should be able to import it by running psql -d cargo_registry -f sample_crates.sql.

I created two users, you and me, and three crates-- one that we each own individually and one that we own jointly. Each crate has only one version.

I didn't add any crates owned by teams since it looks like you're just working on users right now :)

@wlonk
Copy link
Contributor Author

wlonk commented Aug 30, 2016

Thanks, that data was really helpful.

I have placeholder text in the user profile right now; I'm not sure what that page should show. Do you have any suggestions?

@alexcrichton
Copy link
Member

r? @steveklabnik

@steveklabnik
Copy link
Member

Could we get a screenshot?

@wlonk
Copy link
Contributor Author

wlonk commented Sep 1, 2016

Screenshots attached:

screenshot 2016-09-01 10 28 33

screenshot 2016-09-01 10 28 39

screenshot 2016-09-01 10 28 51

import PaginationMixin from '../../mixins/pagination';

const { computed } = Ember;
// TODO: reduce duplicatoin with controllers/crates
Copy link
Member

Choose a reason for hiding this comment

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

small note here; eventually, controllers are going away in Ember, so this might be extra-fine for now :)

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've been trying to train myself out of them, with little success so far!

@steveklabnik
Copy link
Member

Okay, thanks @wlonk ! This looks great overall. Just one question before merging... I wonder what we should put instead of "generic info here."

@wlonk
Copy link
Contributor Author

wlonk commented Sep 2, 2016

Yeah, I asked @carols10cents that upthread, too. I don't really know. We might want to just make the user detail page be their list of crates for now, if we don't have any profile information to show. I can do that tonight if I get a 👍 on it.

@carols10cents
Copy link
Member

Yeah, I think the two pages could totally be merged, and just list someone's crates on their user page. That's all I'd like to know for now!

We don't need a distinct user profile page at the moment.
@wlonk
Copy link
Contributor Author

wlonk commented Sep 2, 2016

And now it looks like this:

screenshot 2016-09-01 20 57 14

And the code is simpler and cleaner as a result.

@wlonk
Copy link
Contributor Author

wlonk commented Sep 13, 2016

@carols10cents bump! I'd love it if you could get a chance to look at this. Thanks!

@carols10cents
Copy link
Member

The user pages look great to me locally!!!

2 things about links:

  • The header on the user pages is a link to... the page we're currently on, which seems redundant. Can we make that not a link? Screenshot of the link i'm talking about:

screen shot 2016-09-15 at 3 21 13 pm

- Is this page linked from anywhere else, such as a crate's page? IMO the "Owners" icons on a crate page should link to the user's (and eventually group's) page instead of to github like it does currently. There's also Authors which currently doesn't link anywhere, but I _think_ this is just the person who published the version you're looking at, so it's not as complete a list as the owners. Screenshot of what I mean:

screen shot 2016-09-15 at 3 24 30 pm

It's really really close!!!

@wlonk
Copy link
Contributor Author

wlonk commented Sep 16, 2016

Changes incorporated. Thanks for shepherding me through this.

I'm curious whether you think it'd be worth adding a link to the user's GitHub page on their Crates profile, perhaps via a GitHub logo somewhere in the profile header area.

@carols10cents
Copy link
Member

a GitHub logo somewhere in the profile header area.

That sounds like a good idea! I personally don't often use a github link to a person/org from crates.io, but I could see cases in which it'd be useful. There's also a desire to have other ways to create a crates.io account without github in the future, so a github logo that could optionally be there or not if github is connected to that person would be awesome (as opposed to making the github link like, the username, or something else that couldn't accommodate multiple links to different places in the future).

@wlonk
Copy link
Contributor Author

wlonk commented Sep 16, 2016

That's what I'm thinking, yeah—the "if there's a linked GitHub profile, show a GitHub logo with that link" and, for now, there will always be a linked GitHub profile.

@wlonk
Copy link
Contributor Author

wlonk commented Sep 17, 2016

OK, I've added a GitHub profile link using their mark. Unfortunately, my local dev setup fell down for reasons that are unclear to me at the moment, so I can't provide a screenshot!

@carols10cents
Copy link
Member

carols10cents commented Sep 18, 2016

Unfortunately, my local dev setup fell down for reasons that are unclear to me at the moment, so I can't provide a screenshot!

i got u fam!

We're getting into the REALLY REALLY picky now: the github logo is smooshing up reaaal close to the username:

needs-padding

I think this could be fixed by adding a similar padding-right to #crates-heading h1 to match its padding-left? That would look like:

has padding

I saved the whole webpage and threw it in a repo to maybe make it easier for you to play with the CSS, maybe? Repo: https://github.com/carols10cents/cratesio-page and rendered gh-pages: https://carols10cents.github.io/cratesio-page/

So incredibly close!! I can't wait til this is deployed!!

@wlonk
Copy link
Contributor Author

wlonk commented Sep 19, 2016

That looks like great alignment to me! Hope this is good to go.

screenshot 2016-09-18 18 58 15

@carols10cents
Copy link
Member

Ok I am r+ on this but I don't actually have the power to r+ ;)

r? @steveklabnik

@wlonk wlonk changed the title [WIP] Show user details. Show user details. Sep 28, 2016
@wlonk
Copy link
Contributor Author

wlonk commented Sep 28, 2016

Bump?

@steveklabnik
Copy link
Member

@bors: r+

I like it too, thanks so much, and sorry about the latency in reviewing.

@steveklabnik
Copy link
Member

Oh wait, we don't use bors on crates.io. 😄

@steveklabnik steveklabnik merged commit 09b3e7b into rust-lang:master Sep 28, 2016
@wlonk
Copy link
Contributor Author

wlonk commented Sep 28, 2016

@steveklabnik It's OK! I just didn't want that open loop to stay open in my head. Thanks for merging!

@wlonk wlonk deleted the user-group-crates-list branch September 28, 2016 19:27
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