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.

[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.

[bjester]

Based off your investigation, I thinking it would be good to have getNodeDetailsErrorsList return a non-empty array for a missing node and thus getContentNodeDetailsAreValid can be simplified

[akolson]

@bjester we should be good after the most recent changes (based on my understanding of your comment above)?

[bjester]

@bjester we should be good after the most recent changes (based on my understanding of your comment above)?

Not quite what I had in mind. I don't quite understand the latest push. Sorry for the miscommunication.

Both getContentNodeDetailsAreValid and getNodeDetailsErrorsList used the utility getNodeDetailsErrors to determine whether the node was valid, but we we had competing definitions of what valid meant internally to those getters. One was defensive against the node missing in the vuex store and also checked NEW_OBJECT, while the other didn't.

Since you were modifying getNodeDetailsErrorsList to be defensive, which was also already extant in getContentNodeDetailsAreValid, the getters were becoming redundant and thus led me to suggest that all that logic should be defined in getNodeDetailsErrorsList, including the existence and the NEW_OBJECT check. Although, to avoid maintaining competing definitions of validity, since your change returned an empty array for a missing node (an OKAY result) while the other getter returned false for a missing node (a NOT OKAY result), I suggest that we return a non-empty array (a new error type) which eliminates the differences between the getters.

Thus moving all that logic into getNodeDetailsErrorsList allows us to modify getContentNodeDetailsAreValid to simply check if the array returned from that getter is empty or not, and consolidates validation logic into one getter getNodeDetailsErrorsList.

[akolson]

Ahh got it--my bad! Thanks for the extra detail!

[akolson]

@bjester, I think we should be good with this? 🤞

[bjester]

LGTM, thanks @akolson !