Adds defensive check when validating node title

[akolson]

Summary

This pr adds a defensive on the code for the error reported in sentry as detailed here.

References

Fixes #5433

Reviewer guidance

• Edit a channel resource
• Ensure no regressions in behavior
• Check logs to ensure above logs are not reported
• A regression test has also been added, so should pass.

[bjester]

Why is an empty node being passed to this validation method to begin with? That seems problematic if the code is expecting to have a node and doesn't. So I wouldn't really expect the defensive code here, but rather defensive against whatever edge case is leading to an undefined node.

[rtibbles]

Flagging that I made a similar comment in the issue itself:

Another defensive programming check needed - we probably should be checking that a node is defined before we run any other checks on it? (No need to add this to each individual validator, hopefully)

If we can avoid running any individual attribute validation in this case, that does seem preferable!

[akolson]

getNodeDetailsErrors calls getNodeTitleErrors and the implementation here allowed passing of undefined content nodes thus the error. The other use case in getContentNodeDetailsAreValid (which also calls the getNodeTitleErrors function) handles this edge case correctly. cc @bjester

[bjester]

This seems better, in that unlike with the previous change, we won't be returning an error about an invalid title when the real error was a missing node. But we're also not returning anything, when a missing node might be problematic. The code in getContentNodeDetailsAreValid would return false with a missing node. This could obscure issues by returning a positive result in a problematic scenario. Adding a new error type might require strings though.

I still wonder if this is happening in a situation that we can actually correct-- one that's caused by code creating this scenario instead of a real scenario a user might encounter. It seems this is only used in the EditView, but getContentNodeDetailsAreValid is also used (I didn't dig deeply). The expectation of a getter like this is that the data has already been loaded into Vuex. Correctly handling that assumption might mitigate this.

So I wonder if this getter is being triggered before the node has been loaded into vuex, which might occur when refreshing the page with the edit modal open, similar to that bug we had when refreshing page with the import modal open. If the result of getContentNodeDetailsAreValid supersedes however this getter is used, then it could return a new error type without us needing to handle the result of that error with new strings.

[akolson]

Thanks for this! I'll do a little more digging in these lines and see what I can find.

[akolson]

It doesn't seem that any preloading of required nodes is happening in the EditModal, a possible reason why this error could be triggering, particularly on refresh. Looking at the routing, we should be able to preload the detailNodeIds prior to entering the Edit modal. This, in addition to the defensive check should mitigate the issue, I think!

studio/contentcuration/contentcuration/frontend/channelEdit/router.js

Lines 174 to 229 in ee0de12

	{
	name: RouteNames.CONTENTNODE_DETAILS,
	path: '/:nodeId/:detailNodeId?/details/:detailNodeIds/:tab?/:targetNodeId?',
	props: true,
	component: EditModal,
	beforeEnter: (to, from, next) => {
	return store
	.dispatch('currentChannel/loadChannel')
	.catch(error => {
	throw new Error(error);
	})
	.then(() => next());
	},
	},
	{
	name: RouteNames.ADD_TOPICS,
	path: '/:nodeId/:detailNodeId?/topics/:detailNodeIds/:tab?',
	props: true,
	component: EditModal,
	beforeEnter: (to, from, next) => {
	return store
	.dispatch('currentChannel/loadChannel')
	.catch(error => {
	throw new Error(error);
	})
	.then(() => next());
	},
	},
	{
	name: RouteNames.ADD_EXERCISE,
	path: '/:nodeId/:detailNodeId?/exercise/:detailNodeIds/:tab?',
	props: true,
	component: EditModal,
	beforeEnter: (to, from, next) => {
	return store
	.dispatch('currentChannel/loadChannel')
	.catch(error => {
	throw new Error(error);
	})
	.then(() => next());
	},
	},
	{
	name: RouteNames.UPLOAD_FILES,
	path: '/:nodeId/:detailNodeId?/upload/:detailNodeIds?/:tab?',
	props: true,
	component: EditModal,
	beforeEnter: (to, from, next) => {
	return store
	.dispatch('currentChannel/loadChannel')
	.catch(error => {
	throw new Error(error);
	})
	.then(() => next());
	},
	},

[bjester]

How about this bit of code? Seems like if the catch occurs, perhaps the following then block might encounter a situation where a node hasn't been loaded. Although that code doesn't utilize any of these getters at first glance, but perhaps it later runs into the bug situation. It might be interesting to see whether there are coinciding network errors (e.g. 502s) in sentry for the users who encountered this.

studio/contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue

Lines 388 to 463 in ee0de12

	beforeRouteEnter(to, from, next) {
	if (
	to.name === RouteNames.CONTENTNODE_DETAILS ||
	to.name === RouteNames.ADD_TOPICS ||
	to.name === RouteNames.ADD_EXERCISE ||
	to.name === RouteNames.UPLOAD_FILES
	) {
	return next(vm => {
	// Catch view-only enters before loading data
	if (!vm.canEdit) {
	return vm.navigateBack();
	}
	vm.loading = true;
	let promises;
	const parentTopicId = to.params.nodeId;
	const childrenNodesIds =
	to.params.detailNodeIds !== undefined ? to.params.detailNodeIds.split(',') : [];
	// remove duplicates - if a topic is being edited,
	// then parent topic ID is also in children nodes IDs
	const allNodesIds = [...new Set([...childrenNodesIds, parentTopicId])];
	// Nice to have TODO: Refactor EditModal to make each tab
	// responsible for fetching data that it needs
	if (childrenNodesIds.length) {
	promises = [
	vm.loadContentNodes({ id__in: allNodesIds }),
	...childrenNodesIds.map(nodeId => vm.loadRelatedResources(nodeId)),
	// Do not remove - there is a logic that relies heavily
	// on assessment items and files being properly loaded
	// (especially marking nodes as (in)complete)
	vm.loadFiles({ contentnode__in: childrenNodesIds }),
	vm.loadAssessmentItems({ contentnode__in: childrenNodesIds }),
	];
	} else {
	// no need to load assessment items or files as topics have none
	promises = [vm.loadContentNode(parentTopicId)];
	}
	return Promise.all(promises)
	.then(() => {
	vm.updateTitleForPage();
	vm.loading = false;
	})
	.catch(() => {
	vm.loading = false;
	vm.loadError = true;
	})
	.then(() => {
	// self-healing of nodes' validation status
	// in case we receive incorrect data from backend
	const validationPromises = [];
	allNodesIds.forEach(nodeId => {
	const node = vm.getContentNode(nodeId);
	const completeCheck = isNodeComplete({
	nodeDetails: node,
	assessmentItems: vm.getAssessmentItems(nodeId),
	files: vm.getContentNodeFiles(nodeId),
	});
	if (completeCheck !== node.complete) {
	validationPromises.push(
	vm.updateContentNode({
	id: nodeId,
	complete: completeCheck,
	checkComplete: true,
	}),
	);
	}
	});
	return Promise.all(validationPromises);
	});
	});
	}
	return next(false);
	},

[akolson]

This seems consistent with all users that triggered the error 🤔

[akolson]

So technically, the defensive check added should resolve the issue, I think? Probably not... would only suppress it!

[akolson]

And yes this seems to happen in refreshing the page, pointing back to to your thoughts on the need to preload the detailNodeIds

[bjester]

Ah sorry, my last comment about it "doesn't utilize any of these getters" was invalid. I was mixing up the two PRs you have open.

Okay so the beforeRouteEnter I linked should preload them, but it also may not complete successfully. It also does some validation but only to mark the nodes' complete. If beforeRouteEnter should preload the nodes into vuex, any insights into where we're going wrong? Are we not properly awaiting something?

[akolson]

The code in the beforeRouteEnter guard seems functionally correct; the issue arises from a Vue reactivity timing problem between parent and child component where getNodeDetailsErrorsList attempts to access state (content nodes data) before it’s fully initialized. However, once the state is ready, the prop re-triggers the UI update, thus the reason why the UI remains functional. The current fix should effectively resolves the issue without regressions, and should be suitable for now. A more robust solution would require extensive planning and work.