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

stop displaying and serving authorship information #1322

Merged
merged 1 commit into from
Mar 22, 2021
Merged

stop displaying and serving authorship information #1322

merged 1 commit into from
Mar 22, 2021

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Mar 19, 2021 •
edited
Loading

@jyn514
Copy link
Member

jyn514 commented Mar 19, 2021

This is wrong, it also prevents viewing pages by the owner. It should only remove get_releases_by_author.

@Rustin170506
Copy link
Member Author

@jyn514 Do we need to rename these api and routing parameters?

Nemo157
Nemo157 reviewed Mar 19, 2021
View reviewed changes
Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

I think it would be good to rename where appropriate, but that seems less important to be done straight away. We also need to stop recording the data we're no longer displaying, and then delete the existing data from the database. I think it would make sense to open a local tracking issue here so those steps can be done independently.

templates/crate/details.html Outdated Show resolved Hide resolved
Rustin170506
Rustin170506 commented Mar 19, 2021
View reviewed changes
src/web/releases.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 requested a review from Nemo157 March 19, 2021 14:23
jyn514
jyn514 reviewed Mar 19, 2021
View reviewed changes
templates/releases/header.html Outdated Show resolved Hide resolved
src/web/releases.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 mentioned this pull request Mar 19, 2021
Closed
3 tasks
@Rustin170506 Rustin170506 requested a review from jyn514 March 19, 2021 15:08
jyn514
jyn514 reviewed Mar 20, 2021
View reviewed changes
src/web/error.rs Outdated Show resolved Hide resolved
src/web/releases.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Mar 20, 2021 •
edited
Loading

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

@jyn514 jyn514 added the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Mar 20, 2021
@Rustin170506
Copy link
Member Author

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

@Rustin170506 Rustin170506 requested a review from jyn514 March 21, 2021 02:02
@Rustin170506
Copy link
Member Author

Rustin170506 commented Mar 21, 2021 •
edited
Loading

@jyn514 Thank you for your review.

@jyn514
Copy link
Member

jyn514 commented Mar 21, 2021

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

Sorry, I meant to say "an author instead of an owner". The existing tests are only for when the owner is found, not when there's an error.

@Rustin170506
Copy link
Member Author

Can you add a test for what happens when you try to request an owner instead of an author? There are some examples near the end of src/web/releases.rs.

I think we already have owners_page and owners_pagination tests.

Sorry, I meant to say "an author instead of an owner". The existing tests are only for when the owner is found, not when there's an error.

Add nonexistent_owner_page for it.

Rustin170506
Rustin170506 commented Mar 21, 2021
View reviewed changes
src/web/releases.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

@jyn514 I think it's ready for review.

@Rustin170506
Copy link
Member Author

@rustbot label: -S-waiting-on-author +S-waiting-on-review

@jyn514
Copy link
Member

jyn514 commented Mar 21, 2021

Add nonexistent_owner_page for it.

Thanks. That shows the error message was wrong: you returned the right error, but the handler ignores it, which was the long-standing bug the comments didn't describe very well. I opened #1326 fixing it, I'd prefer to wait for that before merging.

@jyn514 jyn514 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Mar 21, 2021
@Rustin170506
Copy link
Member Author

Thanks. That shows the error message was wrong: you returned the right error, but the handler ignores it, which was the long-standing bug the comments didn't describe very well. I opened #1326 fixing it, I'd prefer to wait for that before merging.

Got it. Besides this issue, is there anything else that needs to be changed in this PR?

jyn514
jyn514 reviewed Mar 21, 2021
View reviewed changes
src/web/releases.rs Outdated Show resolved Hide resolved
src/web/releases.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 requested a review from jyn514 March 21, 2021 15:14
@jyn514
Copy link
Member

jyn514 commented Mar 21, 2021

@hi-rustin I don't plan to review this again until #1326 is merged. Feel free to ping me if I forget :)

jyn514
jyn514 reviewed Mar 22, 2021
View reviewed changes
templates/rustdoc/topbar.html
@@ -90,21 +90,6 @@
</li>
</ul>
</div>

{# Show the crate authors #}
Copy link
Member

Choose a reason for hiding this comment

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

Could you show the owners instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am currently unable to check the topbar related pages because I am having problems with add-essential-files.
See: rust-lang/rustwide#41 (comment)

So I don't know if the whole topbar is currently displayed properly. Can you help check it? Or do you have any suggestions for solving the above problem.

Copy link
Member

Choose a reason for hiding this comment

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

If you have working IPv6 then https://docs.rs.dev.nemo157.com/ is running this PR currently, looks like it's working to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code, I should have used the correct field.

docs.rs/src/web/crate_details.rs

Line 19 in a0438c4

owners: Vec<(String, String)>,

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have working IPv6 then https://docs.rs.dev.nemo157.com/ is running this PR currently, looks like it's working to me.

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I am having problems with add-essential-files.
See: rust-lang/rustwide#41 (comment)

@jyn514 @Nemo157 Do you have any suggestions to solving this?

templates/crate/details.html Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 22, 2021
@Rustin170506
stop displaying and serving authorship information
68a8517 
revert owners

fix order

refine header comment

test: add cases

fix

add author nope

fix

better msg

fix

rename api and route

owner support without @

refine msg

refine msg

add more cases

add owner

add nonexistent_owner test

rename

fix test

address comments

fix test

fix test

add owners for topbar

fmt code
@Rustin170506 Rustin170506 requested a review from jyn514 March 22, 2021 16:32
jyn514
jyn514 approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@jyn514 jyn514 merged commit de03cb2 into rust-lang:master Mar 22, 2021
@pietroalbini pietroalbini mentioned this pull request Mar 22, 2021
Closed
5 tasks
@Rustin170506 Rustin170506 deleted the rustin-author branch March 23, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

@Nemo157 Nemo157 Awaiting requested review from Nemo157

Assignees
No one assigned
Labels
S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rustin170506 @jyn514 @Nemo157