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

Show right-hand column for yanked versions to crate owners. #1759

Merged
merged 4 commits into from
Aug 30, 2019
Merged

Show right-hand column for yanked versions to crate owners. #1759

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bde7dd
Show right-hand column on yanked version page to crate owners.
smarnach Jun 5, 2019
f383545
Fix indentation.
smarnach Jun 12, 2019
3067604
Merge remote-tracking branch 'origin' into manage-owners-yanked
smarnach Jun 18, 2019
234a9ac
Add a unit test for notYankedOrIsOwner (authored by @efx).
smarnach Jun 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/controllers/crate/version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link

Choose a reason for hiding this comment

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

could we add unit tests for this computed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

@ghost ghost Jun 19, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

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

ember generate controller-test crate/version

Copy link

Choose a reason for hiding this comment

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

I was lazy and didn't fork your copy, but here is the working example of the unit test: https://github.com/rust-lang/crates.io/compare/master...efx:unit-test-controller?expand=1#diff-a30cef8546b1174ebf3c37a550e2357f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

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

return (
!this.get('model').yanked || this.get('crate.owner_user').findBy('id', this.get('session.currentUser.id'))
);
}),

sortedVersions: readOnly('crate.versions'),

Expand Down
Loading