Merge pull request #2161 from umbraco/v14/bugfix/remove-compositions

refactor composition logic

Author: nielslyngsoe

src/packages/core/content-type/structure/content-type-container-structure-helper.class.ts

@@ -1,7 +1,7 @@
 import type { UmbContentTypeModel, UmbPropertyContainerTypes, UmbPropertyTypeContainerModel } from '../types.js';
 import type { UmbContentTypeStructureManager } from './content-type-structure-manager.class.js';
 import { UmbControllerBase } from '@umbraco-cms/backoffice/class-api';
-import type { UmbControllerHost } from '@umbraco-cms/backoffice/controller-api';
+import type { UmbController, UmbControllerHost } from '@umbraco-cms/backoffice/controller-api';
 import { UmbArrayState } from '@umbraco-cms/backoffice/observable-api';
 
 /**
@@ -17,6 +17,8 @@ export class UmbContentTypeContainerStructureHelper<T extends UmbContentTypeMode
 
 	#structure?: UmbContentTypeStructureManager<T>;
 
+	#containerObservers: Array<UmbController> = [];
+
 	// State containing the all containers defined in the data:
 	#childContainers = new UmbArrayState<UmbPropertyTypeContainerModel>([], (x) => x.id);
 	readonly containers = this.#childContainers.asObservable();
@@ -150,30 +152,31 @@ export class UmbContentTypeContainerStructureHelper<T extends UmbContentTypeMode
 				this.#parentType,
 			),
 			(containers) => {
-				// We want to remove hasProperties of groups that does not exist anymore.:
-				// this.#removeHasPropertiesOfGroup()
 				this.#hasProperties.setValue([]);
 				this.#childContainers.setValue([]);
+				this.#containerObservers.forEach((x) => x.destroy());
+				this.#containerObservers = [];
 
 				containers.forEach((container) => {
 					this.#observeHasPropertiesOf(container.id);
 
-					this.observe(
-						this.#structure!.containersOfParentId(container.id, this.#childType!),
-						(containers) => {
-							// get the direct owner containers of this container id:
-							this.#ownerChildContainers =
-								this.#structure!.getOwnerContainers(this.#childType!, this.#containerId!) ?? [];
-							// TODO: Maybe check for dif before setting it? Cause currently we are setting it every time one of the containers change. [NL]
-
-							// Remove existing containers that are not the parent of the new containers:
-							this.#childContainers.filter(
-								(x) => x.parent?.id !== container.id || containers.some((y) => y.id === x.id),
-							);
-
-							this.#childContainers.append(containers);
-						},
-						'_observeGroupsOf_' + container.id,
+					this.#containerObservers.push(
+						this.observe(
+							this.#structure!.containersOfParentId(container.id, this.#childType!),
+							(containers) => {
+								// get the direct owner containers of this container id: [NL]
+								this.#ownerChildContainers =
+									this.#structure!.getOwnerContainers(this.#childType!, this.#containerId!) ?? [];
+
+								// Remove existing containers that are not the parent of the new containers: [NL]
+								this.#childContainers.filter(
+									(x) => x.parent?.id !== container.id || containers.some((y) => y.id === x.id),
+								);
+
+								this.#childContainers.append(containers);
+							},
+							'_observeGroupsOf_' + container.id,
+						),
 					);
 				});
 			},

src/packages/core/content-type/structure/content-type-property-structure-helper.class.ts

@@ -21,7 +21,7 @@ export class UmbContentTypePropertyStructureHelper<T extends UmbContentTypeModel
 
 	#structure?: UmbContentTypeStructureManager<T>;
 
-	private _containerId?: string | null;
+	#containerId?: string | null;
 
 	// State which holds all the properties of the current container, this is a composition of all properties from the containers that matches our target [NL]
 	#propertyStructure = new UmbArrayState<UmbPropertyTypeModel>([], (x) => x.id);
@@ -59,12 +59,12 @@ export class UmbContentTypePropertyStructureHelper<T extends UmbContentTypeModel
 	}
 
 	public setContainerId(value?: string | null) {
-		if (this._containerId === value) return;
-		this._containerId = value;
+		if (this.#containerId === value) return;
+		this.#containerId = value;
 		this.#observeContainers();
 	}
 	public getContainerId() {
-		return this._containerId;
+		return this.#containerId;
 	}
 
 	private _containerName?: string;
@@ -74,9 +74,9 @@ export class UmbContentTypePropertyStructureHelper<T extends UmbContentTypeModel
 
 	#containers?: Array<UmbPropertyTypeContainerModel>;
 	#observeContainers() {
-		if (!this.#structure || this._containerId === undefined) return;
+		if (!this.#structure || this.#containerId === undefined) return;
 
-		if (this._containerId === null) {
+		if (this.#containerId === null) {
 			this.observe(
 				this.#structure.propertyStructuresOf(null),
 				(properties) => {
@@ -87,7 +87,7 @@ export class UmbContentTypePropertyStructureHelper<T extends UmbContentTypeModel
 			this.removeUmbControllerByAlias('_observeContainers');
 		} else {
 			this.observe(
-				this.#structure.containerById(this._containerId),
+				this.#structure.containerById(this.#containerId),
 				(container) => {
 					if (container) {
 						this._containerName = container.name ?? '';

src/packages/core/content-type/structure/content-type-structure-manager.class.ts

@@ -42,10 +42,13 @@ export class UmbContentTypeStructureManager<
 	readonly ownerContentType = this.#contentTypes.asObservablePart((x) =>
 		x.find((y) => y.unique === this.#ownerContentTypeUnique),
 	);
-
-	private readonly _contentTypeContainers = this.#contentTypes.asObservablePart((x) =>
-		x.flatMap((x) => x.containers ?? []),
+	readonly ownerContentTypeCompositions = this.#contentTypes.asObservablePart(
+		(x) => x.find((y) => y.unique === this.#ownerContentTypeUnique)?.compositions,
 	);
+
+	readonly #contentTypeContainers = this.#contentTypes.asObservablePart(() => {
+		return this.#contentTypes.getValue().flatMap((x) => x.containers ?? []);
+	});
 	readonly contentTypeUniques = this.#contentTypes.asObservablePart((x) => x.map((y) => y.unique));
 	readonly contentTypeAliases = this.#contentTypes.asObservablePart((x) => x.map((y) => y.alias));
 
@@ -61,12 +64,12 @@ export class UmbContentTypeStructureManager<
 		super(host);
 		this.#repository = typeRepository;
 
-		this.observe(this.contentTypes, (contentTypes) => {
-			contentTypes.forEach((contentType) => {
-				this._loadContentTypeCompositions(contentType);
-			});
+		// Observe owner content type compositions, as we only allow one level of compositions at this moment. [NL]
+		// But, we could support more, we would just need to flatMap all compositions and make sure the entries are unique and then base the observation on that. [NL]
+		this.observe(this.ownerContentTypeCompositions, (ownerContentTypeCompositions) => {
+			this._loadContentTypeCompositions(ownerContentTypeCompositions);
 		});
-		this.observe(this._contentTypeContainers, (contentTypeContainers) => {
+		this.observe(this.#contentTypeContainers, (contentTypeContainers) => {
 			this.#containers.setValue(contentTypeContainers);
 		});
 	}
@@ -136,8 +139,24 @@ export class UmbContentTypeStructureManager<
 		this._observeContentType(data);
 	}
 
-	private async _loadContentTypeCompositions(contentType: T) {
-		contentType.compositions?.forEach((composition) => {
+	private async _loadContentTypeCompositions(ownerContentTypeCompositions: T['compositions'] | undefined) {
+		if (!ownerContentTypeCompositions) {
+			// Owner content type was undefined, so we can not load compositions. But at this point we neither offload existing compositions, this is most likely not a case that needs to be handled.
+			return;
+		}
+
+		const ownerUnique = this.getOwnerContentTypeUnique();
+		// Remove content types that does not exist as compositions anymore:
+		this.#contentTypes.getValue().forEach((x) => {
+			if (
+				x.unique !== ownerUnique &&
+				!ownerContentTypeCompositions.find((comp) => comp.contentType.unique === x.unique)
+			) {
+				this.#contentTypeObservers.find((y) => y.controllerAlias === 'observeContentType_' + x.unique)?.destroy();
+				this.#contentTypes.removeOne(x.unique);
+			}
+		});
+		ownerContentTypeCompositions.forEach((composition) => {
 			this._ensureType(composition.contentType.unique);
 		});
 	}
@@ -164,23 +183,19 @@ export class UmbContentTypeStructureManager<
 
 		// Notice we do not store the content type in the store here, cause it will happen shortly after when the observations gets its first initial callback. [NL]
 
-		// Load inherited and composed types:
-		//this._loadContentTypeCompositions(data);// Should not be necessary as this will be done when appended to the contentTypes state. [NL]
-
 		const ctrl = this.observe(
 			// Then lets start observation of the content type:
 			await this.#repository.byUnique(data.unique),
 			(docType) => {
 				if (docType) {
-					// TODO: Handle if there was changes made to the owner document type in this context. [NL]
-					/*
-					possible easy solutions could be to notify user wether they want to update(Discard the changes to accept the new ones). [NL]
-					 */
 					this.#contentTypes.appendOne(docType);
+				} else {
+					// Remove the content type from the store, if it does not exist anymore.
+					this.#contentTypes.removeOne(data.unique);
 				}
-				// TODO: Do we need to handle the undefined case? [NL]
 			},
 			'observeContentType_' + data.unique,
+			// Controller Alias is used to stop observation when no longer needed. [NL]
 		);
 
 		this.#contentTypeObservers.push(ctrl);