Fix #467 - Author ordering

[Eh2406]

Rebased and deld the pr by accident. so I resubmitted it.

[carols10cents]

Hi! I'm sorry for the delay in taking a look at this. I think it'd be better if the ordering was done by SQL instead of in Rust, just since ordering things is one of the tasks SQL is really really good at.

Could you change the query for authors to include ORDER BY name ASC instead of the code you have here? Thanks! ❤️

[Eh2406]

I want the sort to be done on what is displayed. What is the value of row.get("name") where row.get("user_id") is some? Is it always the same as Author::User(try!(User::find(conn, id))).name?
If so, we can simplify the authors loop a lot. If not, then we cannot just sort in sql on name.

[Eh2406]

Looks like the only insert to version_authors is at l189 with a TODO: at least try to link name to a pre-existing user and it only inserts a name. So a sort by ORDER BY name ASC will work the same but will brake when the TODO is fixed.

I see 3 fixes:

1. Add a left join to users and a ORDER BY CASE WHEN version_authors.name IS NOT NULL version_authors.name ELSE users.name END ASC Pro: Sort in sql. Con: Complex join for a feature we are not using and a complex sort that sql may not optimize.
2. Do the sort in rust. This is the current pr. Pro: No additional join. Con: Complexity in the rust and using sort, which may not be as fast as sql sort.
3. Make an issue for fixing the TODO. Add the simple ORDER BY name ASC as you suggested, with a TODO that links to the issue to fix it when the other TODO is fixed.

Do you have other suggestions?

[carols10cents]

Ohhhh I understand now. I disagree with that TODO, actually, there's nothing requiring values in authors in Cargo.toml to match with anything in crates.io's database; authors is entirely separate from owners and I don't think it's possible to connect them. There are no rows with a value for user_id in the version_authors table in production, and I don't think anyone has asked for this feature.

So what about a 4th solution:

• Remove the TODO, and the println! right before it while you're in there, there's no reason for that either
• Remove the attempt at using version_authors.user_id and just map rows to Author::Name(row.get("name"))
• Add ORDER BY name to the query

[Eh2406]

If we do that, presumably, we should remove all the Author as User code. By removing the variant of the enum, and everything that references it. Including the JSON response. And whatever consumes that JSON, would that be a breaking change to the API? Maybe we leave the api with a comment that it has no data, but is just there backwards compatibility.

[Eh2406]

Ok I remove the User as Author from the back end. I looked through the front end and did not see any handling of User to be removed, I may have missed something. I left the JSON response with a comment.

[carols10cents]

Looks great!!! Thank you for your patience!!! I don't think we need to worry too much about anything relying on this API, since it never returned any values anyway :) Perhaps we can make an /api/v2/ someday that gets rid of the users key in the json :)

Thank you!!! ❤️

[Eh2406]

a Next Step would be to delete the useless columns in the database, I don't know how to do that. but if you're mucking about with the database tomorrow it might be a good opportunity.

…

On Mar 6, 2017 10:11 PM, "Carol (Nichols || Goulding)" < ***@***.***> wrote: Merged #571 <#571>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#571 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADiaQLH_iUafm3g5e4vqri4jeB23p59qks5rjMrygaJpZM4MKdAJ> .