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

Handling copy failures and retry capability #4060

Closed
wants to merge 3 commits into from
Closed

Handling copy failures and retry capability #4060

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23088ce
Handling copy failures and retry capability
vkWeb May 5, 2023
6b20ff4
Merge branch 'unstable' into handle-fail-copy
vkWeb May 5, 2023
8628934
Fix resources test to include the new error flag
vkWeb May 5, 2023
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
83 changes: 75 additions & 8 deletions ...entcuration/contentcuration/frontend/channelEdit/components/ContentNodeListItem/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,24 @@
<template v-else>
<div class="copying">
<p class="caption grey--text pr-2 pt-1">
{{ $tr("copyingTask") }}
<span :style="{ 'cursor': hasCopyingErrored ? 'default' : 'progress' }">
{{ copyingMessage }}
</span>
<span v-if="hasCopyingErrored">
<ActionLink :text="$tr('retryCopy')" @click="retryFailedCopy" />
</span>
</p>
<TaskProgress :taskId="taskId" size="30" />
<ContentNodeCopyTaskProgress
:node="node"
size="30"
/>
<IconButton
v-if="hasCopyingErrored"
icon="close"
:text="$tr('removeNode')"
size="small"
@click="removeFailedCopyNode"
/>
</div>
<div class="disabled-overlay"></div>
</template>
Expand All @@ -190,8 +205,9 @@

<script>

import { mapActions } from 'vuex';
import camelCase from 'lodash/camelCase';
import TaskProgress from '../../views/progress/TaskProgress';
import ContentNodeCopyTaskProgress from '../../views/progress/ContentNodeCopyTaskProgress';
import ContentNodeChangedIcon from '../ContentNodeChangedIcon';
import ContentNodeValidator from '../ContentNodeValidator';
import { ContentLevels, Categories, NEW_OBJECT } from 'shared/constants';
Expand All @@ -203,9 +219,11 @@
import ContextMenuCloak from 'shared/views/ContextMenuCloak';
import DraggableHandle from 'shared/views/draggable/DraggableHandle';
import { titleMixin, metadataTranslationMixin } from 'shared/mixins';
import { COPYING_FLAG, TASK_ID } from 'shared/data/constants';
import { COPYING_FLAG, COPYING_ERROR_FLAG, RELATIVE_TREE_POSITIONS } from 'shared/data/constants';
import { EffectAllowed } from 'shared/mixins/draggable/constants';
import ContentNodeLearningActivityIcon from 'shared/views/ContentNodeLearningActivityIcon';
import { ContentNode } from 'shared/data/resources';
import { withChangeTracker } from 'shared/data/changes';

export default {
name: 'ContentNodeListItem',
Expand All @@ -217,7 +235,7 @@
ContentNodeValidator,
ContentNodeChangedIcon,
ToggleText,
TaskProgress,
ContentNodeCopyTaskProgress,
ContentNodeLearningActivityIcon,
},
mixins: [titleMixin, metadataTranslationMixin],
Expand Down Expand Up @@ -292,8 +310,15 @@
copying() {
return this.node[COPYING_FLAG];
},
taskId() {
return this.node[TASK_ID];
hasCopyingErrored() {
return this.node[COPYING_ERROR_FLAG];
},
copyingMessage() {
if (this.hasCopyingErrored) {
return this.$tr('copyingError');
} else {
return this.$tr('copyingTask');
}
},
},
watch: {
Expand All @@ -307,6 +332,41 @@
},
},
methods: {
...mapActions(['showSnackbar', 'clearSnackbar']),
...mapActions('contentNode', ['deleteContentNode', 'copyContentNode']),
removeFailedCopyNode() {
return this.deleteContentNode(this.node.id);
},
retryFailedCopy: withChangeTracker(async function(changeTracker) {
const original_source_node = await ContentNode.table
.where('node_id')
.equals(this.node.original_source_node_id)
.toArray();
this.showSnackbar({
duration: null,
text: this.$tr('creatingCopies'),
});

return this.copyContentNode({
id: original_source_node[0]['id'],
target: this.node.id,
position: RELATIVE_TREE_POSITIONS.RIGHT,
wait_for_status: true,
startingRev: changeTracker._startingRev,
is_retry: true,
})
.then(() => {
this.showSnackbar({
text: this.$tr('copiedSnackbar'),
actionText: this.$tr('undo'),
actionCallback: () => changeTracker.revert(),
}).then(() => changeTracker.cleanUp());
})
.catch(() => {
this.clearSnackbar();
changeTracker.cleanUp();
});
}),
vkWeb marked this conversation as resolved.
Show resolved Hide resolved
handleTileClick(e) {
// Ensures that clicking an icon button is not treated the same as clicking the card
if (e.target && e.target.tagName !== 'svg' && !this.copying) {
Expand Down Expand Up @@ -353,6 +413,12 @@
'{value, number, integer} {value, plural, one {resource for coaches} other {resources for coaches}}',
coachTooltip: 'Resource for coaches',
copyingTask: 'Copying',
copyingError: 'Copy failed.',
removeNode: 'Remove',
retryCopy: 'Retry',
creatingCopies: 'Copying...',
copiedSnackbar: 'Copy operation complete',
undo: 'Undo',
},
};

Expand Down Expand Up @@ -398,8 +464,9 @@
.copying {
z-index: 2;
display: flex;
align-items: baseline;
padding-top: 44px;
cursor: progress;
pointer-events: all;

p,
div {
Expand Down
14 changes: 9 additions & 5 deletions contentcuration/contentcuration/frontend/channelEdit/components/ContentNodeOptions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import { mapActions, mapGetters } from 'vuex';
import { RouteNames, TabNames } from '../constants';
import MoveModal from './move/MoveModal';
import { ContentNode } from 'shared/data/resources';
import { withChangeTracker } from 'shared/data/changes';
import { RELATIVE_TREE_POSITIONS } from 'shared/data/constants';

Expand Down Expand Up @@ -104,7 +103,7 @@
},
},
methods: {
...mapActions(['showSnackbar']),
...mapActions(['showSnackbar', 'clearSnackbar']),
...mapActions('contentNode', ['createContentNode', 'moveContentNodes', 'copyContentNode']),
...mapActions('clipboard', ['copy']),
newTopicNode() {
Expand Down Expand Up @@ -187,19 +186,24 @@
// actionText: this.$tr('cancel'),
// actionCallback: () => changeTracker.revert(),
});

return this.copyContentNode({
id: this.nodeId,
target: this.nodeId,
position: RELATIVE_TREE_POSITIONS.RIGHT,
}).then(node => {
ContentNode.waitForCopying([node.id]).then(() => {
wait_for_status: true,
})
.then(() => {
this.showSnackbar({
text: this.$tr('copiedSnackbar'),
actionText: this.$tr('undo'),
actionCallback: () => changeTracker.revert(),
}).then(() => changeTracker.cleanUp());
})
.catch(() => {
this.clearSnackbar();
changeTracker.cleanUp();
});
});
}),
trackAction(eventLabel) {
this.$analytics.trackAction('channel_editor_node', 'Click', {
Expand Down
15 changes: 6 additions & 9 deletions contentcuration/contentcuration/frontend/channelEdit/components/StudioTree/StudioTree.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,11 @@
style="width: 24px;"
shrink
>
<TaskProgress
<ContentNodeCopyTaskProgress
class="progress-loader"
:taskId="taskId"
:node="node"
size="24"
showTooltip
/>
</VFlex>
<VFlex
Expand Down Expand Up @@ -185,7 +186,7 @@
import ContentNodeChangedIcon from '../ContentNodeChangedIcon';
import ContentNodeValidator from '../ContentNodeValidator';
import ContentNodeContextMenu from '../ContentNodeContextMenu';
import TaskProgress from '../../views/progress/TaskProgress';
import ContentNodeCopyTaskProgress from '../../views/progress/ContentNodeCopyTaskProgress';
import { ContentKindsNames } from 'shared/leUtils/ContentKinds';
import ContextMenuCloak from 'shared/views/ContextMenuCloak';
import LoadingText from 'shared/views/LoadingText';
Expand All @@ -194,7 +195,7 @@
import DraggableItem from 'shared/views/draggable/DraggableItem';
import DraggableHandle from 'shared/views/draggable/DraggableHandle';
import { titleMixin } from 'shared/mixins';
import { COPYING_FLAG, TASK_ID } from 'shared/data/constants';
import { COPYING_FLAG } from 'shared/data/constants';
import { DropEffect, EffectAllowed } from 'shared/mixins/draggable/constants';
import { objectValuesValidator } from 'shared/mixins/draggable/utils';

Expand All @@ -211,7 +212,7 @@
ContentNodeValidator,
LoadingText,
IconButton,
TaskProgress,
ContentNodeCopyTaskProgress,
},
mixins: [titleMixin],
inject: ['draggableUniverse'],
Expand Down Expand Up @@ -303,9 +304,6 @@
copying() {
return this.node && this.node[COPYING_FLAG];
},
taskId() {
return this.node && this.node[TASK_ID];
},
activeDropEffect() {
// Don't allow dropping into itself
const { metadata } = this.deepestActiveDraggable || {};
Expand Down Expand Up @@ -470,7 +468,6 @@
z-index: 2;
margin: auto 0;
pointer-events: all;
cursor: progress;
}

.node-item {
Expand Down
33 changes: 22 additions & 11 deletions contentcuration/contentcuration/frontend/channelEdit/views/CurrentTopicView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@
import { DraggableTypes, DropEffect } from 'shared/mixins/draggable/constants';
import { DraggableFlags } from 'shared/vuex/draggablePlugin/module/constants';
import DraggableRegion from 'shared/views/draggable/DraggableRegion';
import { ContentNode } from 'shared/data/resources';

export default {
name: 'CurrentTopicView',
Expand Down Expand Up @@ -420,7 +419,7 @@
},
},
methods: {
...mapActions(['showSnackbar']),
...mapActions(['showSnackbar', 'clearSnackbar']),
...mapActions(['setViewMode', 'addViewModeOverride', 'removeViewModeOverride']),
...mapActions('contentNode', [
'createContentNode',
Expand Down Expand Up @@ -636,23 +635,35 @@
// actionText: this.$tr('cancel'),
// actionCallback: () => changeTracker.revert(),
});
return Promise.all(
id__in.map(id =>
this.copyContentNode({
this.clearSelections();

return Promise.allSettled(
id__in.map(id => {
return this.copyContentNode({
id,
target: id,
position: RELATIVE_TREE_POSITIONS.RIGHT,
})
)
).then(nodes => {
this.clearSelections();
ContentNode.waitForCopying(nodes.map(n => n.id)).then(() => {
wait_for_status: true,
});
})
).then(results => {
let anyErrored = false;
for (const result of results) {
if (result.status === 'rejected') {
anyErrored = true;
break;
}
}
if (anyErrored) {
this.clearSnackbar();
changeTracker.cleanUp();
} else {
this.showSnackbar({
text: this.$tr('copiedItems'),
actionText: this.$tr('undo'),
actionCallback: () => changeTracker.revert(),
}).then(() => changeTracker.cleanUp());
});
}
});
}),
scroll(e) {
Expand Down
33 changes: 18 additions & 15 deletions ...contentcuration/frontend/channelEdit/views/ImportFromChannels/ImportFromChannelsModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@
this.updateTitleForPage();
},
methods: {
...mapActions('contentNode', ['copyContentNodes']),
...mapActions('contentNode', ['copyContentNode']),
...mapMutations('importFromChannels', {
selectNode: 'SELECT_NODE',
deselectNode: 'DESELECT_NODE',
Expand Down Expand Up @@ -209,20 +209,23 @@
});
},
handleClickImport() {
const nodeIds = this.selected.map(({ id }) => id);
return this.copyContentNodes({
id__in: nodeIds,
target: this.$route.params.destNodeId,
}).then(() => {
// When exiting, do not show snackbar when clearing selections
this.showSnackbar = false;
this.$store.commit('importFromChannels/CLEAR_NODES');
this.$router.push({
name: RouteNames.TREE_VIEW,
params: {
nodeId: this.$route.params.destNodeId,
},
});
Promise.allSettled(
this.selected.map(({ id }) => {
return this.copyContentNode({
id,
target: this.$route.params.destNodeId,
wait_for_status: true,
});
})
);
Copy link
Member

Choose a reason for hiding this comment

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

Nothing is using the resulting promise from Promise.allSettled

Copy link
Member Author

Choose a reason for hiding this comment

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

Using Promise.allSettled so that the rejections from the underlying promise doesn't result in console errors. Because here we don't need to handle rejected promises from copyContentNode. Does it make sense sir?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I use an empty catch here? (maybe a better alternative)

// When exiting, do not show snackbar when clearing selections
this.showSnackbar = false;
this.$store.commit('importFromChannels/CLEAR_NODES');
this.$router.push({
name: RouteNames.TREE_VIEW,
params: {
nodeId: this.$route.params.destNodeId,
},
});
},
// Using a click method here instead of :to attribute because
Expand Down
Loading