Newest updated #1931

bruceadams · 2019-12-01T01:40:52Z

This PR is working toward addressing #310. This is incomplete. There is a relevant test commented out because it fails.

rust-highfive · 2019-12-01T01:40:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jtgeibel (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bruceadams · 2019-12-01T02:07:53Z

This is pretty far from correct 😞 … some discussion on #310.

bruceadams · 2019-12-01T15:20:26Z

I'm much happier with this code. Pretty much everywhere that used to have some concept of max_version now has top_versions which contains both the highest semver version and the most recently updated version.

What is not yet here is using the most recently updated version in the user interface. (I don't really know where this is in the code, but I haven't really looked either.)

carols10cents

Love it! So to finish this up, the frontend for the home page will need to be updated:

For the Just Updated column, the newest version should be displayed
For the Most Downloaded column, no version should be displayed (as discussed in the issue, if you agree)
For the New Crates column, the highest version should be displayed (current behavior)

And the search result/crate list pages should also keep the current behavior of showing the highest version.

There's a frontend test for the front page that's very minimal right now but should be updated to test the new logic.

Currently, the home page template calls the crate-list component, which always displays the max version. I'm not sure the best way to change what's displayed in the various cases, but that's where some change will need to be :)

src/models/krate.rs

src/tests/util.rs

src/controllers/krate/metadata.rs

src/models/version.rs

bors · 2019-12-02T18:50:49Z

☔ The latest upstream changes (presumably #1891) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel · 2019-12-03T02:49:01Z

This looks really good to me so far! A few things I noticed running locally:

The Just Updated column isn't showing a version, just empty (). I think this may be causing the test failure as well.
Everything still links to the top-level crates/crate_name route. When a version is displayed, it would be nice if the link went directly to that version's page.

To test the end-to-end behavior, I'm looking to publish the following set of crates to staging (Cargo.toml below). Let me know if you think of any additional edge cases that should be covered.

[workspace]

# The comments show the intended publish order
members = [
    "prerelease_only",        # 0.0.1-alpha.0
    "prerelease_then_normal", # 0.0.1-alpha.0, 0.0.1
    "normal_then_prerelease", # 0.0.1, 0.1.0-alpha.0
    "normal_release_only",    # 0.0.1
    "normal_releases",        # 0.0.1, 0.1.0, 0.1.1, 0.2.0
    "normal_and_backport",    # 0.1.0, 0.2.0, 0.2.1, 0.1.1
    "normal_and_backport2",   # 0.1.0, 0.2.0, 0.1.1, 0.2.1
]

bruceadams · 2019-12-03T14:11:44Z

I see the empty version for Just Updated and struggled to understand how max_version worked and newest_version did not. I think I have that detail fixed at this point 🎉. I like suggestion about linking straight to a specific version when we display a specific version. I may not get to that today.

carols10cents

Just 2 small questions about consistency. Tried it out locally and it's working!

app/templates/index.hbs

app/templates/components/crate-list-newest.hbs

bruceadams · 2019-12-08T00:00:07Z

So… I'm happy with the state of the code and the user interface in this pull request as it currently stands. The main thing I'm uncomfortable about is that the scope of this change has grown well beyond the original issue #310. I am more than willing to trim this back to a narrow scope that only fiddles with the Just Updated column and leaves everything else as it is today. I can imagine some in between places as well. Preferences?

bruceadams · 2019-12-11T12:21:13Z

Sorry about the test failure 😞. Running the front end test locally stopped working and I haven't figured out why.

carols10cents

I left a comment for my guess about why the tests might be failing for you locally; if that's not it, I'd be happy to investigate with you (I'll be at Rust Coffee tonight, will you?)

Looking at the failures in Percy, most are expected because of the changes this PR makes to the UI. Are you able to see the Percy diffs? I forget how that works. If not, let me know and I'll post the screenshots here.

The one Percy failure I'm not sure about is for the Acceptance | crate page | is accessible test; the total number of downloads on the homepage is different. That seems brittle to me because this PR didn't change anything about that part of the page. That test is using the factories, though, and this PR did change the factories, so perhaps we've been relying on coincidence.

On further investigation, I'm not really sure why this test exists; it appears to be testing the home page, but we already have this test for the homepage (that uses the fixtures instead of the factories, so it should be more stable).

Are you up for a tiny bit more scope creep on this PR, just deleting that one crate-test? :)

carols10cents · 2019-12-11T14:42:31Z

tests/acceptance/crate-test.js

@@ -66,7 +66,7 @@ module('Acceptance | crate page', function(hooks) {
    await visit('/');
    await click('[data-test-just-updated] [data-test-crate-link="0"]');

-    assert.equal(currentURL(), '/crates/nanomsg');
+    assert.equal(currentURL(), '/crates/nanomsg/6.8.9');


Is this test one that's failing for you locally? I think it's because this test uses the factories, and the version number isn't guaranteed to be the same every time the factory gets called?

This can be fixed by explicitly setting a newest_version for this particular instance, sort of like this test explicitly sets a max_version.

Running the tests locally crashes fairly badly. I don't have much experience with npm stuffs, so I don't have much clue what is going on. It did work for me. I don't know of having changed anything (although I'm a compulsive brew upgrader and that might've broken something).

Yikes! I have occasional luck with removing node_modules and re-npm installing? We can look at it tonight though :)

Locally, I've replaced this test with the following. This should eliminate the dependence on faker data and make the test more robust to future changes. Would you add a final commit with this or a similar update?

test('visiting a crate page from the front page', async function(assert) { this.server.create('crate', { id: 'nanomsg', newest_version: '0.6.1' }); this.server.create('version', { crate: 'nanomsg', num: '0.6.1' }); await visit('/'); await click('[data-test-just-updated] [data-test-crate-link="0"]'); assert.equal(currentURL(), '/crates/nanomsg/0.6.1'); assert.equal(title(), 'nanomsg - crates.io: Rust Package Registry'); assert.dom('[data-test-heading] [data-test-crate-name]').hasText('nanomsg'); assert.dom('[data-test-heading] [data-test-crate-version]').hasText('0.6.1'); });

bruceadams · 2019-12-11T17:03:17Z

I will be at the Rust coffee this evening 🎉 @jtgeibel and I looked at the Percy failures. It seems to be related to auto generated stuff that has interesting cross cutting impact. In some bigger picture, it'd be great to stabilize that stuff.

I'm fine with test cleanup. I'm not going to be able to really look at code for several hours.

bors · 2019-12-20T12:48:44Z

☔ The latest upstream changes (presumably #2015) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2019-12-21T07:13:21Z

☔ The latest upstream changes (presumably #2017) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2019-12-22T01:28:44Z

☔ The latest upstream changes (presumably #2044) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel

@bruceadams thanks for the recent rebases! I've finally had a chance to take another pass at this and things are looking really good. I've added a comment with one final suggestion. I'll also accept the graphical changes in percy.

jtgeibel · 2019-12-22T03:13:06Z

tests/acceptance/crate-test.js

@@ -66,7 +66,7 @@ module('Acceptance | crate page', function(hooks) {
    await visit('/');
    await click('[data-test-just-updated] [data-test-crate-link="0"]');

-    assert.equal(currentURL(), '/crates/nanomsg');
+    assert.equal(currentURL(), '/crates/nanomsg/6.8.9');


Locally, I've replaced this test with the following. This should eliminate the dependence on faker data and make the test more robust to future changes. Would you add a final commit with this or a similar update?

test('visiting a crate page from the front page', async function(assert) { this.server.create('crate', { id: 'nanomsg', newest_version: '0.6.1' }); this.server.create('version', { crate: 'nanomsg', num: '0.6.1' }); await visit('/'); await click('[data-test-just-updated] [data-test-crate-link="0"]'); assert.equal(currentURL(), '/crates/nanomsg/0.6.1'); assert.equal(title(), 'nanomsg - crates.io: Rust Package Registry'); assert.dom('[data-test-heading] [data-test-crate-name]').hasText('nanomsg'); assert.dom('[data-test-heading] [data-test-crate-version]').hasText('0.6.1'); });

bruceadams · 2019-12-22T13:21:03Z

@jtgeibel Your test adjustment makes sense; I added it. The first rebase yesterday took me a while! I was happy the second one was easy!

bors · 2019-12-22T19:37:24Z

☔ The latest upstream changes (presumably #2049) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel · 2019-12-23T00:31:17Z

Thanks @bruceadams! I hate to say it, but this needs another rebase now, hopeful another easy one though. If you get around to it today, ping me and I'll r+ ASAP. There's been a lot of work on the frontend recently, sorry for the high rate of churn.

This contains far too much repeated code. It does basically work and the relevant test passes! I'll refactor the redundant code into a shared function.

Where we used to have max_version, we now have two kinds of top versions: - the highest semver version - the most recently updated version.

- For the Just Updated column, the newest version is displayed - For the Most Downloaded column, no version is displayed - For the New Crates column, the highest version is displayed (unchanged)

bruceadams · 2019-12-23T14:47:17Z

I learned more about Ember.js, including that the documentation is very good even for a reader like me who barely has a clue about Ember.js.

jtgeibel · 2019-12-23T15:25:52Z

Thanks Bruce!

@bors: r+

bors · 2019-12-23T15:25:54Z

📌 Commit fb356c4 has been approved by jtgeibel

Newest updated This PR is working toward addressing #310. This is incomplete. There is a relevant test commented out because it fails.

bors · 2019-12-23T15:26:01Z

⌛ Testing commit fb356c4 with merge 45b8dbf...

bors · 2019-12-23T15:32:35Z

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing 45b8dbf to master...

rust-highfive assigned jtgeibel Dec 1, 2019

rust-highfive added the S-waiting-on-review label Dec 1, 2019

bruceadams mentioned this pull request Dec 1, 2019

Just Updated column lists greatest, not the latest version #310

Closed

carols10cents requested changes Dec 2, 2019

View reviewed changes

src/models/krate.rs Show resolved Hide resolved

src/tests/util.rs Show resolved Hide resolved

src/controllers/krate/metadata.rs Show resolved Hide resolved

src/models/version.rs Show resolved Hide resolved

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Dec 2, 2019

bruceadams force-pushed the newest-updated branch from caec94e to 5d0fe88 Compare December 3, 2019 00:18

bruceadams force-pushed the newest-updated branch from 05a7948 to 11327ae Compare December 5, 2019 01:22

carols10cents requested changes Dec 5, 2019

View reviewed changes

app/templates/index.hbs Outdated Show resolved Hide resolved

app/templates/components/crate-list-newest.hbs Outdated Show resolved Hide resolved

carols10cents requested changes Dec 11, 2019

View reviewed changes

bruceadams force-pushed the newest-updated branch from 550f63d to 218c3eb Compare December 13, 2019 13:48

bruceadams force-pushed the newest-updated branch from 218c3eb to f07b9ef Compare December 20, 2019 13:18

bruceadams force-pushed the newest-updated branch from f07b9ef to 57460ba Compare December 21, 2019 21:49

bruceadams force-pushed the newest-updated branch from 57460ba to 4103c8f Compare December 22, 2019 02:52

jtgeibel requested changes Dec 22, 2019

View reviewed changes

bruceadams added 16 commits December 23, 2019 05:37

Sketch testing newest_version

e7a3609

Add newest field

592f612

TopVersions for highest and newest

f65a640

Doc for top_versions function

4d92475

Deal with TopVersions everywhere

ec7a166

This contains far too much repeated code. It does basically work and the relevant test passes! I'll refactor the redundant code into a shared function.

Refactor top_version into the version module

55af1d1

Where we used to have max_version, we now have two kinds of top versions: - the highest semver version - the most recently updated version.

Fancy handling of crate versions on home page

0b373d7

- For the Just Updated column, the newest version is displayed - For the Most Downloaded column, no version is displayed - For the New Crates column, the highest version is displayed (unchanged)

Set nanomsg-0.6.1 as newest, not highest

797fce0

Add newest_version to each crate fixture

8df1061

Add newest_version placeholders

d02aeb9

For "Just Updated" link to the specific version

759a920

Adjust tests for versioned "Just Updated" links

2e085bf

Even fewer crate versions on home page

1b02232

For "New Crates" link to the specific version shown

79af4e4

Finish reverting the "New Crates" test for version

66f9ab6

Narrow test fixtures in single test

77f0849

bruceadams force-pushed the newest-updated branch from 045b0e8 to 77f0849 Compare December 23, 2019 10:47

Repair LinkTo and this. missed during rebase

fb356c4

bors added a commit that referenced this pull request Dec 23, 2019

Auto merge of #1931 - bruceadams:newest-updated, r=jtgeibel

45b8dbf

Newest updated This PR is working toward addressing #310. This is incomplete. There is a relevant test commented out because it fails.

bors merged commit fb356c4 into rust-lang:master Dec 23, 2019

This was referenced Dec 23, 2019

Add Crate Owner Auditing #2025

Closed

Add a concrete util::Error type (step 3 of refactoring) #2032

Merged

jtgeibel mentioned this pull request Jan 18, 2020

Linking directly to a new version from the front page shows no downloads #2143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Newest updated #1931

Newest updated #1931

bruceadams commented Dec 1, 2019

rust-highfive commented Dec 1, 2019

bruceadams commented Dec 1, 2019

bruceadams commented Dec 1, 2019

carols10cents left a comment

bors commented Dec 2, 2019

jtgeibel commented Dec 3, 2019

bruceadams commented Dec 3, 2019

carols10cents left a comment

bruceadams commented Dec 8, 2019

bruceadams commented Dec 11, 2019

carols10cents left a comment

carols10cents Dec 11, 2019

bruceadams Dec 11, 2019

carols10cents Dec 11, 2019

jtgeibel Dec 22, 2019

bruceadams commented Dec 11, 2019

bors commented Dec 20, 2019

bors commented Dec 21, 2019

bors commented Dec 22, 2019

jtgeibel left a comment

jtgeibel Dec 22, 2019

bruceadams commented Dec 22, 2019

bors commented Dec 22, 2019

jtgeibel commented Dec 23, 2019

bruceadams commented Dec 23, 2019

jtgeibel commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019

Newest updated #1931

Newest updated #1931

Conversation

bruceadams commented Dec 1, 2019

rust-highfive commented Dec 1, 2019

bruceadams commented Dec 1, 2019

bruceadams commented Dec 1, 2019

carols10cents left a comment

Choose a reason for hiding this comment

bors commented Dec 2, 2019

jtgeibel commented Dec 3, 2019

bruceadams commented Dec 3, 2019

carols10cents left a comment

Choose a reason for hiding this comment

bruceadams commented Dec 8, 2019

bruceadams commented Dec 11, 2019

carols10cents left a comment

Choose a reason for hiding this comment

carols10cents Dec 11, 2019

Choose a reason for hiding this comment

bruceadams Dec 11, 2019

Choose a reason for hiding this comment

carols10cents Dec 11, 2019

Choose a reason for hiding this comment

jtgeibel Dec 22, 2019

Choose a reason for hiding this comment

bruceadams commented Dec 11, 2019

bors commented Dec 20, 2019

bors commented Dec 21, 2019

bors commented Dec 22, 2019

jtgeibel left a comment

Choose a reason for hiding this comment

jtgeibel Dec 22, 2019

Choose a reason for hiding this comment

bruceadams commented Dec 22, 2019

bors commented Dec 22, 2019

jtgeibel commented Dec 23, 2019

bruceadams commented Dec 23, 2019

jtgeibel commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019