Skip to content

simplify props of cloud-manager-hoc #5198

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 1 commit into from
Jan 28, 2020
Merged

simplify props of cloud-manager-hoc #5198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 16 additions & 12 deletions src/lib/cloud-manager-hoc.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,10 @@ const cloudManagerHOC = function (WrappedComponent) {
canUseCloud (props) {
return !!(props.cloudHost && props.username && props.vm && props.projectId && props.hasCloudPermission);
}
shouldNotModifyCloudData (props) {
return (props.hasEverEnteredEditor && !props.canSave);
}
shouldConnect (props) {
return !this.isConnected() && this.canUseCloud(props) &&
props.isShowingWithId && props.vm.runtime.hasCloudData() &&
!this.shouldNotModifyCloudData(props);
props.canModifyCloudData;
}
shouldDisconnect (props, prevProps) {
return this.isConnected() &&
Expand All @@ -70,7 +67,7 @@ const cloudManagerHOC = function (WrappedComponent) {
(props.projectId !== prevProps.projectId) ||
(props.username !== prevProps.username) ||
// Editing someone else's project
this.shouldNotModifyCloudData(props)
!props.canModifyCloudData
);
}
isConnected () {
Expand Down Expand Up @@ -102,11 +99,11 @@ const cloudManagerHOC = function (WrappedComponent) {
render () {
const {
/* eslint-disable no-unused-vars */
canModifyCloudData,
cloudHost,
projectId,
username,
hasCloudPermission,
hasEverEnteredEditor,
isShowingWithId,
onShowCloudInfo,
/* eslint-enable no-unused-vars */
Expand All @@ -124,23 +121,30 @@ const cloudManagerHOC = function (WrappedComponent) {
}

CloudManager.propTypes = {
canSave: PropTypes.bool.isRequired,
canModifyCloudData: PropTypes.bool.isRequired,
Copy link
Contributor Author

@benjiwheeler benjiwheeler Jan 27, 2020

Choose a reason for hiding this comment

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

note that I switched the boolean value of shouldNotModifyCloudData canModifyCloudData.

There's potential confusion with hasCloudPermission, which in www is set to isScratcher; I have ideas for further simplification here, but I think we should take it one step at a time.

cloudHost: PropTypes.string,
hasCloudPermission: PropTypes.bool,
hasEverEnteredEditor: PropTypes.bool,
isShowingWithId: PropTypes.bool,
isShowingWithId: PropTypes.bool.isRequired,
onShowCloudInfo: PropTypes.func,
projectId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
username: PropTypes.string,
vm: PropTypes.instanceOf(VM).isRequired
};

const mapStateToProps = state => {
CloudManager.defaultProps = {
cloudHost: null,
hasCloudPermission: false,
onShowCloudInfo: () => {},
username: null
};

const mapStateToProps = (state, ownProps) => {
const loadingState = state.scratchGui.projectState.loadingState;
return {
hasEverEnteredEditor: state.scratchGui.mode.hasEverEnteredEditor,
isShowingWithId: getIsShowingWithId(loadingState),
projectId: state.scratchGui.projectState.projectId
projectId: state.scratchGui.projectState.projectId,
// if you're editing someone else's project, you can't modify cloud data
canModifyCloudData: (!state.scratchGui.mode.hasEverEnteredEditor || ownProps.canSave)
};
};

Expand Down
45 changes: 2 additions & 43 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 @@ -60,7 +60,6 @@ describe('CloudManagerHOC', () => {

mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -80,7 +79,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
store={store}
username="user"
Expand All @@ -98,7 +96,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -115,7 +112,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={stillLoadingStore}
Expand All @@ -132,7 +128,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
cloudHost="nonEmpty"
hasCloudPermission={false}
store={store}
Expand All @@ -153,7 +148,6 @@ describe('CloudManagerHOC', () => {

const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={stillLoadingStore}
Expand Down Expand Up @@ -182,7 +176,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={stillLoadingStore}
Expand All @@ -209,7 +202,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -235,7 +227,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -262,7 +253,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand Down Expand Up @@ -293,7 +283,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -315,7 +304,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand Down Expand Up @@ -343,7 +331,6 @@ describe('CloudManagerHOC', () => {
const WrappedComponent = cloudManagerHOC(Component);
mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -364,12 +351,11 @@ describe('CloudManagerHOC', () => {
});

// Editor Mode Connection/Disconnection Tests
test('Entering editor mode and can\'t save project should disconnect cloud provider # 1', () => {
test('Entering editor mode and can\'t save project should disconnect cloud provider', () => {
const Component = () => <div />;
const WrappedComponent = cloudManagerHOC(Component);
const mounted = mount(
<WrappedComponent
canSave
hasCloudPermission
cloudHost="nonEmpty"
store={store}
Expand All @@ -382,34 +368,7 @@ describe('CloudManagerHOC', () => {
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
hasCloudPermission
canSave={false}
cloudHost="nonEmpty"
store={store}
username="user"
vm={vm}
/>
);

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

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

expect(vm.setCloudProvider.mock.calls.length).toBe(2);
Expand Down