Re-add setting sourceUrl to null in uploaded images #20795
Conversation
|
Note: I'm working on a PR with a small UI test suite to check for breaking changes to gamelab animations. Will probably end up merging that to staging before this one. |
| // TODO (bbuchanan): This sequencing feels backwards. Eventually, we | ||
| // ought to preview and get dimensions from the local filesystem, async | ||
| // with the upload itself, but that will mean refactoring away from the | ||
| // jQuery uploader. |
There was a problem hiding this comment.
I tore out this comment in #20713 - let's not reintroduce it.
|
|
||
| var onMetadataLoaded = animationPickerModule.buildOnImageMetadataLoaded( | ||
| "filename.jpg", Goal.NEW_ANIMATION, "filename.jpg", | ||
| {filename: "filename.jpg", result: 0, versionId: "string"}, store.dispatch); |
There was a problem hiding this comment.
Style nit: It may seem verbose, but when we have to wrap arguments like this we often just place one argument per line.
var onMetadataLoaded = animationPickerModule.buildOnMetadataLoaded(
"filename.jpg",
Goal.NEW_ANIMATION,
"filename.jpg",
{
filename: "filename.jpg",
result: 0,
versionId: "string"
},
store.dispatch
);| } | ||
| dispatch(hide()); | ||
| }, () => { | ||
| const onImageMetadataLoaded = buildOnImageMetadataLoaded(uploadFilename, goal, key, result, dispatch); |
There was a problem hiding this comment.
Reviewing now, I'm questioning our choice of arguments to this method:
keyis derived fromresultand its only use is to be passed into this method, which also receivesresult. Maybe we should computekeywithin thebuildOnImageMetadataLoadedfunction.- Likewise,
uploadFilenameandgoalaren't actually needed in this method and they should still be in the state when theonImageMetadataLoadedcallback fires.
It's possible want we really want here is to call
buildOnImageMetadataLoaded(dispatch, getState, result);which echoes the signature and redux-thunk return value of handleUploadComplete, and then derive the other values in our helper. In fact, that might make it easier to test that we derive those values correctly.
It does mean you'd need to make sure the animationPicker module's redux state is set up correctly in your unit test.
There was a problem hiding this comment.
Moving the look-up of goal and uploadFilename makes a lot of sense for organization and I believe the test already supports this state-wise, so that'll be quick and easy to change.
For 'key', since it's used to look-up the sourceUrl in handleUploadComplete, I think it makes more sense to still compute it there and pass it into buildOnImageMetadataLoaded. Thoughts?
There was a problem hiding this comment.
👍 My bad, missed the transitive dependency there.
…-org/code-dot-org into gl-anim-loss-upload-url
After, this PR and its revert, adding back making uploaded images have null sourceUrls. Including additional unit test. Not including the version re-routing which caused the error initially. We are still not able to handle situations where uploaded images have a non-null sourceUrl and a version, but we will no longer create images with that state.