Skip to content

Fix assertion error in Ember for version downloads #1728

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

Merged
merged 1 commit into from
May 13, 2019

Conversation

bryanburgers
Copy link
Contributor

@bryanburgers bryanburgers commented Apr 22, 2019

Fix the following assertion error that occurs when running the frontend in debug mode, which cause graphs to not display at all.

Assertion Failed: You must include an 'id' for version-download in an object passed to 'push'

This has been happening since 3c92ca2 landed.

The fix involves telling Ember how to create a unique ID for a version-download object, using the composite key.

I haven't been able to confirm, but I believe this is the reason that graphs in production are only ever counting/displaying a single version. When using this change locally in debug mode against the live website, the downloads graphs are rendering correctly.

Fixes #1704.

Fix the following assertion error that occurs when running the frontend
in debug mode, which cause graphs to not display at all.

> Assertion Failed: You must include an 'id' for version-download in an
> object passed to 'push'

This has been happening since 3c92ca2
landed.

The fix involves telling Ember how to create a unique ID for a
version-download object, using the composite key.

I haven't been able to confirm, but I believe this is the reason that
graphs in production are only ever counting/displaying a single version.
When using this change locally in debug mode against the live website,
the downloads graphs are rendering correctly.

Fixes rust-lang#1704.
@bryanburgers bryanburgers force-pushed the fix-ember-assertion-error branch from 8f6390c to 4b6685e Compare April 22, 2019 19:04
@Turbo87 Turbo87 requested a review from sgrif April 22, 2019 19:53
@kzys
Copy link
Contributor

kzys commented Apr 23, 2019

Looks good to me. Can we write tests around the serializers? How about testing normalize() method?

@Turbo87
Copy link
Member

Turbo87 commented Apr 23, 2019

@kzys I agree that having tests for this would be a good idea. I'm not sure that unit testing the serializer is the best approach though. Maybe we can find a way to acceptance-test this to make sure that it works from end-to-end. (well, without the actual backend though...)

@kzys
Copy link
Contributor

kzys commented Apr 23, 2019

Until we have the way to write end-to-end tests, I'd like to keep some tests against this serializer. Having unit tests helps us to identify root causes quickly, while it tends to couple with the implementation.

@bryanburgers
Copy link
Contributor Author

I don’t typically write much Ember, and I didn’t see any existing tests testing serializers that I could use an example. I’d welcome some advice on how to add useful tests to this!

@sgrif
Copy link
Contributor

sgrif commented May 13, 2019

I agree it'd be great to add more end-to-end tests here, but getting that set up is a large enough task for its own PR. I've confirmed this does fix a major UI bug locally, so I'm going to merge this.

@bors: r+

@bors
Copy link
Contributor

bors commented May 13, 2019

📌 Commit 4b6685e has been approved by sgrif

@bors
Copy link
Contributor

bors commented May 13, 2019

⌛ Testing commit 4b6685e with merge 0d47f35...

bors added a commit that referenced this pull request May 13, 2019
Fix assertion error in Ember for version downloads

Fix the following assertion error that occurs when running the frontend in debug mode, which cause graphs to not display at all.

> Assertion Failed: You must include an 'id' for version-download in an object passed to 'push'

This has been happening since 3c92ca2 landed.

The fix involves telling Ember how to create a unique ID for a version-download object, using the composite key.

I haven't been able to confirm, but I believe this is the reason that graphs in production are only ever counting/displaying a single version. When using this change locally in debug mode against the live website, the downloads graphs are rendering correctly.

Fixes #1704.
@bors
Copy link
Contributor

bors commented May 13, 2019

☀️ Test successful - checks-travis
Approved by: sgrif
Pushing 0d47f35 to master...

@bors bors merged commit 4b6685e into rust-lang:master May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downloads statistics seem broken
6 participants