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

Jump to bottom

Re-add setting sourceUrl to null in uploaded images #20795

Merged
merged 8 commits into from Feb 27, 2018
Merged

Re-add setting sourceUrl to null in uploaded images #20795

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4c4a4d2
Undo version routing change that caused errors
Feb 20, 2018
e2e2c4c
Test that sourceUrl is null in uploaded images
Feb 21, 2018
d842b40
Merge branch 'staging' into gl-anim-loss-upload-url
Feb 21, 2018
416bb35
Styling changes and removed redundent parameters
Feb 21, 2018
bc1f391
Merge branch 'staging' into gl-anim-loss-upload-url
Feb 21, 2018
349d7c2
Test callback in upload image sets sourceUrl null
islemaster Feb 26, 2018
c450815
Merge branch 'staging' into gl-anim-loss-upload-url
Feb 26, 2018
2924bbe
Merge branch 'gl-anim-loss-upload-url' of https://github.com/code-dot…
Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 23 additions & 16 deletions apps/src/gamelab/AnimationPicker/animationPickerModule.js
View file
Open in desktop
Expand Up @@ -110,27 +110,34 @@ export function handleUploadComplete(result) {
const { goal, uploadFilename } = getState().animationPicker;
const key = result.filename.replace(/\.png$/i, '');
const sourceUrl = animationsApi.basePath(key + '.png');

loadImageMetadata(sourceUrl, metadata => {
const animation = _.assign({}, metadata, {
name: uploadFilename,
sourceUrl: sourceUrl,
size: result.size,
version: result.versionId
});

if (goal === Goal.NEW_ANIMATION) {
dispatch(addAnimation(key, animation));
} else if (goal === Goal.NEW_FRAME) {
dispatch(appendCustomFrames(animation));
}
dispatch(hide());
}, () => {
const onImageMetadataLoaded = buildOnImageMetadataLoaded(uploadFilename, goal, key, result, dispatch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing now, I'm questioning our choice of arguments to this method:

  • key is derived from result and its only use is to be passed into this method, which also receives result. Maybe we should compute key within the buildOnImageMetadataLoaded function.
  • Likewise, uploadFilename and goal aren't actually needed in this method and they should still be in the state when the onImageMetadataLoaded callback 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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 My bad, missed the transitive dependency there.

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tore out this comment in #20713 - let's not reintroduce it.

loadImageMetadata(sourceUrl, onImageMetadataLoaded, () => {
dispatch(handleUploadError(gamelabMsg.animationPicker_failedToParseImage()));
});
};
}

export function buildOnImageMetadataLoaded(uploadFilename, goal, key, result, dispatch) {
return (metadata) => {
const animation = _.assign({}, metadata, {
name: uploadFilename,
sourceUrl: null,
size: result.size,
version: result.versionId
});
if (goal === Goal.NEW_ANIMATION) {
dispatch(addAnimation(key, animation));
} else if (goal === Goal.NEW_FRAME) {
dispatch(appendCustomFrames(animation));
}
dispatch(hide());
};
}

/**
* Asynchronously loads an image file as an Image, then derives appropriate
* animation metadata from that Image and returns the metadata to a callback.
Expand Down
23 changes: 23 additions & 0 deletions apps/test/unit/gamelab/AnimationPicker/animationPickerModuleTest.js
View file
Open in desktop
@@ -1,4 +1,7 @@
import reducer, * as animationPickerModule from '@cdo/apps/gamelab/AnimationPicker/animationPickerModule';
import listReducer from '@cdo/apps/gamelab/animationListModule';
import {combineReducers} from 'redux';
import {createStore} from '../../../util/redux';
import {expect} from '../../../util/configuredChai';
var Goal = animationPickerModule.Goal;

Expand Down Expand Up @@ -102,5 +105,25 @@ describe('animationPickerModule', function () {
expect(newState.uploadError).to.equal(status);
});
});

describe('action: handleUploadComplete', function () {

it('sets sourceUrl to null', function () {
const newState = {animationPicker: { uploadFilename: "filename.jpg", goal: Goal.NEW_ANIMATION }};
var store = createStore(combineReducers({animationList: listReducer, animationPicker: reducer}), newState);

var onMetadataLoaded = animationPickerModule.buildOnImageMetadataLoaded(
"filename.jpg", Goal.NEW_ANIMATION, "filename.jpg",
{filename: "filename.jpg", result: 0, versionId: "string"}, store.dispatch);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

onMetadataLoaded({});

const newListState = store.getState().animationList;
const animationKey = newListState.orderedKeys[0];

expect(newListState.propsByKey[animationKey]).to.be.not.null;
expect(newListState.propsByKey[animationKey].name).to.equal("filename.jpg_1");
expect(newListState.propsByKey[animationKey].sourceUrl).to.be.null;
});
});
});
});