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

Re-AMDifies Khan Exercises utils files that had not been properly encoded #5105

Merged
merged 1 commit into from Apr 28, 2016

Conversation

Projects
None yet
3 participants
@rtibbles
Member

rtibbles commented Apr 26, 2016

Summary

Reruns deAMDify on the Khan Exercise utils file. Some of these seemed to be malformed, or possibly even empty.

They are now properly deAMDified and have been confirmed to have content.

Reviewer guidance

Pull and manually test problematic exercises.

Issues addressed

Fixes #5036 and #5099 - and many other exercises that were broken.

Documentation

If the PR has documentation, link the file here (either .rst in your repo or if built on Read The Docs)

Screenshots (if appropriate)

image

image

image

@rtibbles rtibbles added this to the 0.16.4 milestone Apr 26, 2016

@rtibbles rtibbles added the has PR label Apr 26, 2016

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 26, 2016

Member

Will do a manual test to verify this and merge once confirmed that existing issues are fixed.

Once merged, will release as 0.16.4.

Member

benjaoming commented Apr 26, 2016

Will do a manual test to verify this and merge once confirmed that existing issues are fixed.

Once merged, will release as 0.16.4.

@jamalex

This comment has been minimized.

Show comment
Hide comment
@jamalex

jamalex Apr 26, 2016

Member

!!!!!

Member

jamalex commented Apr 26, 2016

!!!!!

@jamalex

This comment has been minimized.

Show comment
Hide comment
@jamalex

jamalex Apr 26, 2016

Member

Once merged, will release as 0.16.4.

The release to end all releases.

Member

jamalex commented Apr 26, 2016

Once merged, will release as 0.16.4.

The release to end all releases.

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 27, 2016

Member

It solves parts of the issues, but it seems now that a host name is missing in URLs for loading data:

/learn/khan/math/geometry/transformations/hs-geo-rotations/rotations-1/

JS traceback:

Data load failed: "http:/content/assessment/khan/e98/e98730da75ea202f048b860cb802424c71818cf1-data.json" undefined bundle_exercise.js:748:513

Occurs on this page. The interaction works, but I would assume that the shape that's supposed to be displayed is rendered through the JSON data that fails to load.

screenshot from 2016-04-27 12 57 10

Member

benjaoming commented Apr 27, 2016

It solves parts of the issues, but it seems now that a host name is missing in URLs for loading data:

/learn/khan/math/geometry/transformations/hs-geo-rotations/rotations-1/

JS traceback:

Data load failed: "http:/content/assessment/khan/e98/e98730da75ea202f048b860cb802424c71818cf1-data.json" undefined bundle_exercise.js:748:513

Occurs on this page. The interaction works, but I would assume that the shape that's supposed to be displayed is rendered through the JSON data that fails to load.

screenshot from 2016-04-27 12 57 10

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 27, 2016

Member

Actually from the above example, /content/assessment/khan/e98/e98730da75ea202f048b860cb802424c71818cf1-data.json does not exist in any case.

Member

benjaoming commented Apr 27, 2016

Actually from the above example, /content/assessment/khan/e98/e98730da75ea202f048b860cb802424c71818cf1-data.json does not exist in any case.

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 27, 2016

Member

Similar issues in many other exercises.

/learn/khan/math/geometry/quadrilaterals-and-polygons/quadrilaterals/quadrilateral_angles/

JS traceback:

Data load failed: "http:/content/assessment/khan/b3c/b3cc297e950300ff2a0b83cd5bacdfda35ee3472-data.json" undefined bundle_exercise.js:748:513
Member

benjaoming commented Apr 27, 2016

Similar issues in many other exercises.

/learn/khan/math/geometry/quadrilaterals-and-polygons/quadrilaterals/quadrilateral_angles/

JS traceback:

Data load failed: "http:/content/assessment/khan/b3c/b3cc297e950300ff2a0b83cd5bacdfda35ee3472-data.json" undefined bundle_exercise.js:748:513

@benjaoming benjaoming assigned rtibbles and unassigned benjaoming Apr 27, 2016

@rtibbles

This comment has been minimized.

Show comment
Hide comment
@rtibbles

rtibbles Apr 27, 2016

Member

Hrm - curiously enough, I do have these files in my content folder, and still get the same error.

Member

rtibbles commented Apr 27, 2016

Hrm - curiously enough, I do have these files in my content folder, and still get the same error.

@rtibbles

This comment has been minimized.

Show comment
Hide comment
@rtibbles

rtibbles Apr 27, 2016

Member

Oh, no, I have the one you note above, but many others are missing.

Member

rtibbles commented Apr 27, 2016

Oh, no, I have the one you note above, but many others are missing.

@rtibbles

This comment has been minimized.

Show comment
Hide comment
@rtibbles

rtibbles Apr 27, 2016

Member

@benjaoming I redownloaded the English content pack, and it cleared up this issue for the exercise you identified. Can you try the same and double check the other ones you found problematic?

Member

rtibbles commented Apr 27, 2016

@benjaoming I redownloaded the English content pack, and it cleared up this issue for the exercise you identified. Can you try the same and double check the other ones you found problematic?

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 28, 2016

Member

@rtibbles what concerns me is this:

http:/content/assessment/khan/
     ^ where is the host name or why not remove 'http:' ?

There seems to be both a lack of error handling, but also an unintended behavior. I have now tested with the "latest" assessment items pack, but it's the same result, and I wouldn't expect otherwise. The file exists, but the request is mal-formatted.

Member

benjaoming commented Apr 28, 2016

@rtibbles what concerns me is this:

http:/content/assessment/khan/
     ^ where is the host name or why not remove 'http:' ?

There seems to be both a lack of error handling, but also an unintended behavior. I have now tested with the "latest" assessment items pack, but it's the same result, and I wouldn't expect otherwise. The file exists, but the request is mal-formatted.

@rtibbles

This comment has been minimized.

Show comment
Hide comment
@rtibbles

rtibbles Apr 28, 2016

Member

Does the item fail to render as a result?

On Thu, 28 Apr 2016, 06:06 Benjamin Bach, notifications@github.com wrote:

@rtibbles https://github.com/rtibbles what concerns me is this:

http:/content/assessment/khan/
^ where is the host name?

There seems to be both a lack of error handling, but also an unintended
behavior. I have now tested with the "latest" assessment items pack, but
it's the same result, and I wouldn't expect otherwise. The file exists, but
the request is mal-formatted.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5105 (comment)

Member

rtibbles commented Apr 28, 2016

Does the item fail to render as a result?

On Thu, 28 Apr 2016, 06:06 Benjamin Bach, notifications@github.com wrote:

@rtibbles https://github.com/rtibbles what concerns me is this:

http:/content/assessment/khan/
^ where is the host name?

There seems to be both a lack of error handling, but also an unintended
behavior. I have now tested with the "latest" assessment items pack, but
it's the same result, and I wouldn't expect otherwise. The file exists, but
the request is mal-formatted.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5105 (comment)

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming
Member

benjaoming commented Apr 28, 2016

screenshot from 2016-04-28 23 14 53

@rtibbles

This comment has been minimized.

Show comment
Hide comment
@rtibbles

rtibbles Apr 28, 2016

Member

All of the questions rendered fine for me after updating the content pack,
I am unable to replicate this.

The fetching is internal to Perseus, so we cannot catch the error in
question.

On Thu, 28 Apr 2016, 14:15 Benjamin Bach, notifications@github.com wrote:

[image: screenshot from 2016-04-28 23 14 53]
https://cloud.githubusercontent.com/assets/374612/14901686/0a71c26c-0d97-11e6-8431-8e6653bc8eaa.png


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5105 (comment)

Member

rtibbles commented Apr 28, 2016

All of the questions rendered fine for me after updating the content pack,
I am unable to replicate this.

The fetching is internal to Perseus, so we cannot catch the error in
question.

On Thu, 28 Apr 2016, 14:15 Benjamin Bach, notifications@github.com wrote:

[image: screenshot from 2016-04-28 23 14 53]
https://cloud.githubusercontent.com/assets/374612/14901686/0a71c26c-0d97-11e6-8431-8e6653bc8eaa.png


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5105 (comment)

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 28, 2016

Member

BINGO! I got it working by using a content pack rather than the legacy assessment items whose contents must have drifted (although their date stamps were the same).

I'll generate new assessment items manually tomorrow unless someone else beats me to it. I will release after and draft a release announcement to share.

Member

benjaoming commented Apr 28, 2016

BINGO! I got it working by using a content pack rather than the legacy assessment items whose contents must have drifted (although their date stamps were the same).

I'll generate new assessment items manually tomorrow unless someone else beats me to it. I will release after and draft a release announcement to share.

@benjaoming benjaoming merged commit b60b1fc into learningequality:0.16.x Apr 28, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@benjaoming benjaoming removed the has PR label Apr 28, 2016

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 29, 2016

Member

I should have approached this differently:

  1. FIrstly, I ran a diff on everything in the content pack and legacy assessment item zip. They are the same, data is fine.
  2. Then, I found out that the unpack_assessment_zip command puts the assessment items in the wrong location.

Should be fixable quite easily... doing that now...

Member

benjaoming commented Apr 29, 2016

I should have approached this differently:

  1. FIrstly, I ran a diff on everything in the content pack and legacy assessment item zip. They are the same, data is fine.
  2. Then, I found out that the unpack_assessment_zip command puts the assessment items in the wrong location.

Should be fixable quite easily... doing that now...

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 29, 2016

Member

Error was that the original assessment_zip used to contain a file channel.name and the legacy version doesn't. Fixing it in the codebase.

Member

benjaoming commented Apr 29, 2016

Error was that the original assessment_zip used to contain a file channel.name and the legacy version doesn't. Fixing it in the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment