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 right-hand column for yanked versions to crate owners. #1759

Merged
merged 4 commits into from Aug 30, 2019

Conversation

@smarnach
Copy link
Contributor

commented Jun 5, 2019

Fixes #1660.

Without this change, the page for a yanked version looks like this:

image

With this change applied, crate owners see this page instead:

image

@smarnach smarnach force-pushed the smarnach:manage-owners-yanked branch from df74424 to c7823b4 Jun 5, 2019

Show right-hand column on yanked version page to crate owners.
Among other things, this makes the "Manage owners" link available even if all versions of a crate were yanked.

@smarnach smarnach force-pushed the smarnach:manage-owners-yanked branch from c7823b4 to 3bde7dd Jun 5, 2019

@smarnach smarnach changed the title Show right-hand column on version page to crate owners. Show right-hand column for yanked versions to crate owners. Jun 5, 2019

app/templates/crate/version.hbs Outdated Show resolved Hide resolved
@@ -33,6 +33,11 @@ export default Controller.extend({
isOwner: computed('crate.owner_user', 'session.currentUser.id', function() {
return this.get('crate.owner_user').findBy('id', this.get('session.currentUser.id'));
}),
notYankedOrIsOwner: computed('model', 'crate.owner_user', 'session.currentUser.id', function() {

This comment has been minimized.

Copy link
@efx

efx Jun 17, 2019

could we add unit tests for this computed?

This comment has been minimized.

Copy link
@smarnach

smarnach Jun 19, 2019

Author Contributor

Sure we could, but I'm not familiar with Ember or Mirage, so I'm not sure what's the best way to do this. I've added an attempt as a new commit that tries to use Mirage to set up a new crate and a version. The test passes in its current form, but has a few problems:

  • I was only able to use the Mirage factories when running the whole application with setupApplicationTest(). This seems unnecessary and makes the test much slower than it should be.
  • Testing the other cases (yanked crate, with the user being owner or not) requires also faking an active session and user accounts. I couldn't find any existing infrastructure to do so in a unit test.

We may be able to test this with an acceptance test instead, but at least for me this would involve significant effort. Maybe someone more experienced with Ember could add this test?

This comment has been minimized.

Copy link
@efx

efx Jun 19, 2019

Apologies @smarnach, I should've added a bit more direction here. Ember can generate default unit tests that should avoid needing to use mirage. The problems you point out highlight the many concerns of acceptance / integration testing I hoped to avoid by suggesting a unit test. I'll open a PR against your branch with a simple unit test.

This comment has been minimized.

Copy link
@efx

efx Jun 19, 2019

I'm doing this in my fork, but you can create test files using ember generate commands.

ember generate controller-test crate/version

This comment has been minimized.

Copy link
@efx

This comment has been minimized.

Copy link
@smarnach

smarnach Jun 19, 2019

Author Contributor

Thanks a lot, this looks a lot more reasonable than my attempt! I guess I should learn a bit more about Ember before doing any more work on the frontend. :)

I've copied your test to this branch (and force-pushed, since my attempt isn't worth being preserved in the Git history).

This comment has been minimized.

Copy link
@efx

efx Jun 19, 2019

certainly @smarnach. Eh, learning on the go seems most efficient 😄. Grand!

@locks locks self-assigned this Jun 18, 2019

@smarnach smarnach force-pushed the smarnach:manage-owners-yanked branch from 4656a9e to 234a9ac Jun 19, 2019

@kzys

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Looks good to me.

@smarnach

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

@locks @kzys Is there anything I can do to move this forward? It would be great if we could get this merged.

@efx

This comment has been minimized.

Copy link

commented Aug 29, 2019

@locks any objections to merging this?

@smarnach smarnach referenced this pull request Aug 30, 2019
@locks
locks approved these changes Aug 30, 2019
@carols10cents

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

The text wrapping is different now if you're not an owner, but I'm fine with it.

Screen Shot 2019-08-30 at 4 46 00 PM

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

📌 Commit 234a9ac has been approved by carols10cents

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

⌛️ Testing commit 234a9ac with merge 27bd9a8...

bors added a commit that referenced this pull request Aug 30, 2019
Auto merge of #1759 - smarnach:manage-owners-yanked, r=carols10cents
Show right-hand column for yanked versions to crate owners.

Fixes #1660.

Without this change, the page for a yanked version looks like this:

![image](https://user-images.githubusercontent.com/249196/58944345-ec823d00-8781-11e9-9322-019250a6fb1d.png)

With this change applied, crate owners see this page instead:

![image](https://user-images.githubusercontent.com/249196/58944390-0de32900-8782-11e9-9c58-ac649c9c23c0.png)
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 27bd9a8 to master...

@bors bors merged commit 234a9ac into rust-lang:master Aug 30, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
percy/crates.io Visual review automatically approved, no visual changes found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.