Skip to content

Disconnect from cloud when entering editor mode on someone else's project #3890

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

Merged
merged 4 commits into from
Nov 29, 2018
Merged

Disconnect from cloud when entering editor mode on someone else's project #3890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e752ffd
Disconnect from cloud when viewing someone else's project in editor mode
kchadha Nov 27, 2018
143dd5e
Add unit tests for disconnecting from cloud when viewing a project in…
kchadha Nov 27, 2018
9fc6d85
Merge branch 'develop' into editor-cloud-disconnect
kchadha Nov 28, 2018
2ca5190
Clean up merge conflict resolution.
kchadha Nov 28, 2018
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
16 changes: 12 additions & 4 deletions src/lib/cloud-manager-hoc.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,24 @@ const cloudManagerHOC = function (WrappedComponent) {
// This should cover info about the website specifically, like scrather status
return !!(props.cloudHost && props.username && props.vm && props.projectId);
}
shouldNotModifyCloudData (props) {
Copy link
Contributor Author

@kchadha kchadha Nov 27, 2018

Choose a reason for hiding this comment

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

This is not a great name, but writing this function so that the name is not a negative check makes the code less readable...

return (props.hasEverEnteredEditor && !props.canSave);
}
shouldConnect (props) {
return !this.isConnected() && this.canUseCloud(props) &&
props.isShowingWithId && props.vm.runtime.hasCloudData();
props.isShowingWithId && props.vm.runtime.hasCloudData() &&
!this.shouldNotModifyCloudData(props);
}
shouldDisconnect (props, prevProps) {
return this.isConnected() &&
( // Can no longer use cloud or cloud provider info is now stale
!this.canUseCloud(props) ||
!props.vm.runtime.hasCloudData() ||
(props.projectId !== prevProps.projectId) ||
(props.username !== prevProps.username)
(props.username !== prevProps.username) ||
// Editing someone else's project
this.shouldNotModifyCloudData(props)
);
// TODO need to add provisions for viewing someone
// else's project in editor mode
}
isConnected () {
return this.cloudProvider && !!this.cloudProvider.connection;
Expand Down Expand Up @@ -98,6 +102,7 @@ const cloudManagerHOC = function (WrappedComponent) {
cloudHost,
projectId,
username,
hasEverEnteredEditor,
isShowingWithId,
/* eslint-enable no-unused-vars */
vm,
Expand All @@ -114,7 +119,9 @@ const cloudManagerHOC = function (WrappedComponent) {
}

CloudManager.propTypes = {
canSave: PropTypes.bool.isRequired,
cloudHost: PropTypes.string,
hasEverEnteredEditor: PropTypes.bool,
isShowingWithId: PropTypes.bool,
projectId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
username: PropTypes.string,
Expand All @@ -124,6 +131,7 @@ const cloudManagerHOC = function (WrappedComponent) {
const mapStateToProps = state => {
const loadingState = state.scratchGui.projectState.loadingState;
return {
hasEverEnteredEditor: state.scratchGui.mode.hasEverEnteredEditor,
isShowingWithId: getIsShowingWithId(loadingState),
projectId: state.scratchGui.projectState.projectId
};
Expand Down
8 changes: 6 additions & 2 deletions src/reducers/gui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ const initPlayer = function (currentState) {
currentState,
{mode: {
isFullScreen: currentState.mode.isFullScreen,
isPlayerOnly: true
isPlayerOnly: true,
// When initializing in player mode, make sure to reset
// hasEverEnteredEditorMode
hasEverEnteredEditor: false
}}
);
};
Expand All @@ -72,7 +75,8 @@ const initFullScreen = function (currentState) {
currentState,
{mode: {
isFullScreen: true,
isPlayerOnly: currentState.mode.isPlayerOnly
isPlayerOnly: currentState.mode.isPlayerOnly,
hasEverEnteredEditor: currentState.mode.hasEverEnteredEditor
}}
);
};
Expand Down
6 changes: 4 additions & 2 deletions src/reducers/mode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ const SET_PLAYER = 'scratch-gui/mode/SET_PLAYER';

const initialState = {
isFullScreen: false,
isPlayerOnly: false
isPlayerOnly: false,
hasEverEnteredEditor: true
};

const reducer = function (state, action) {
Expand All @@ -17,7 +18,8 @@ const reducer = function (state, action) {
case SET_PLAYER:
return {
isFullScreen: state.isFullScreen,
isPlayerOnly: action.isPlayerOnly
isPlayerOnly: action.isPlayerOnly,
hasEverEnteredEditor: state.hasEverEnteredEditor || !action.isPlayerOnly
};
default:
return state;
Expand Down
69 changes: 69 additions & 0 deletions test/unit/util/cloud-manager-hoc.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ describe('CloudManagerHOC', () => {
projectState: {
projectId: '1234',
loadingState: LoadingState.SHOWING_WITH_ID
},
mode: {
hasEverEnteredEditor: false
}
}
});
Expand All @@ -36,6 +39,9 @@ describe('CloudManagerHOC', () => {
projectState: {
projectId: '1234',
loadingState: LoadingState.LOADING_WITH_ID
},
mode: {
hasEverEnteredEditor: false
}
}
});
Expand All @@ -52,6 +58,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -68,6 +75,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
store={store}
username="user"
vm={vm}
Expand All @@ -84,6 +92,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
vm={vm}
Expand All @@ -99,6 +108,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={stillLoadingStore}
username="user"
Expand All @@ -114,6 +124,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={stillLoadingStore}
username="user"
Expand All @@ -134,6 +145,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={stillLoadingStore}
username="user"
Expand All @@ -159,6 +171,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -183,6 +196,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -208,6 +222,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand Down Expand Up @@ -237,6 +252,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -256,6 +272,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -279,6 +296,7 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
Expand All @@ -295,6 +313,57 @@ describe('CloudManagerHOC', () => {
expect(vm.setCloudProvider.mock.calls.length).toBe(2);
expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
expect(requestCloseConnection).toHaveBeenCalledTimes(1);
});

// Editor Mode Connection/Disconnection Tests
test('Entering editor mode and can\'t save project should disconnect cloud provider # 1', () => {
const Component = () => <div />;
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
store={store}
username="user"
vm={vm}
/>
);

expect(CloudProvider).toHaveBeenCalled();
const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;

mounted.setProps({
canSave: false,
hasEverEnteredEditor: true
});

expect(vm.setCloudProvider.mock.calls.length).toBe(2);
expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
expect(requestCloseConnection).toHaveBeenCalledTimes(1);
});

test('Entering editor mode and can\'t save project should disconnect cloud provider # 2', () => {
const Component = () => <div />;
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave={false}
cloudHost="nonEmpty"
store={store}
username="user"
vm={vm}
/>
);

expect(CloudProvider).toHaveBeenCalled();
const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;

mounted.setProps({
hasEverEnteredEditor: true
});

expect(vm.setCloudProvider.mock.calls.length).toBe(2);
expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
expect(requestCloseConnection).toHaveBeenCalledTimes(1);
});
});