Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

sebbi08 · 2025-01-27T08:37:39Z

Readiness checklist

I added/updated tests.
I ensured that the PR title is good enough for the changelog.
I labeled the PR.
I self-reviewed the PR.

…is deleted

codecov · 2025-01-27T08:39:09Z

Codecov Report

Attention: Patch coverage is 85.48387% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...eteThirdPartyRelationshipAttributeAndNotifyPeer.ts	73.33%	4 Missing ⚠️
...ttributes/DeleteOwnSharedAttributeAndNotifyPeer.ts	86.66%	2 Missing ⚠️
...ributes/DeletePeerSharedAttributeAndNotifyOwner.ts	88.23%	2 Missing ⚠️
...ransport/src/modules/messages/MessageController.ts	87.50%	1 Missing ⚠️

Files with missing lines	Coverage Δ
...kages/runtime/src/useCases/common/RuntimeErrors.ts	`84.25% <100.00%> (+0.32%)`	⬆️
...ransport/src/modules/messages/MessageController.ts	`87.79% <87.50%> (ø)`
...ttributes/DeleteOwnSharedAttributeAndNotifyPeer.ts	`92.30% <86.66%> (-1.54%)`	⬇️
...ributes/DeletePeerSharedAttributeAndNotifyOwner.ts	`92.50% <88.23%> (-1.35%)`	⬇️
...eteThirdPartyRelationshipAttributeAndNotifyPeer.ts	`92.59% <73.33%> (-4.47%)`	⬇️

... and 2 files with indirect coverage changes

jkoenig134 · 2025-01-28T14:57:36Z

with #404 this can be defenitely closed as we can just send these messages right away now

…ip is still in Pending state

sebbi08 · 2025-01-31T10:43:42Z

with #404 this can be defenitely closed as we can just send these messages right away now

Sadly, the messages cannot be encrypted until the relationship got accepted by both Peers

jkoenig134 · 2025-01-31T10:45:52Z

?

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

packages/transport/src/modules/messages/MessageController.ts

packages/runtime/src/useCases/common/RuntimeErrors.ts

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

...me/src/useCases/consumption/attributes/DeleteThirdPartyRelationshipAttributeAndNotifyPeer.ts

packages/runtime/test/consumption/attributes.test.ts

packages/transport/src/modules/relationships/RelationshipsController.ts

packages/runtime/test/consumption/attributes.test.ts

packages/transport/src/modules/relationships/RelationshipsController.ts

packages/runtime/test/consumption/attributes.test.ts

packages/transport/src/modules/messages/MessageController.ts

Co-authored-by: Britta Stallknecht <146106656+britsta@users.noreply.github.com>

packages/runtime/test/consumption/attributes.test.ts

packages/runtime/test/lib/testUtils.ts

Co-authored-by: Milena Czierlinski <146972016+Milena-Czierlinski@users.noreply.github.com>

jkoenig134 · 2025-02-04T12:49:29Z

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts

+        const relationshipToPeer = await this.relationshipsController.getRelationshipToIdentity(ownSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);
+
+        if (relationshipToPeer) {
+            return Result.fail(RuntimeErrors.attributes.cannotDeleteSharedAttributeBecausePeerCannotBeNotified());
+        }
+
        const validationResult = await this.attributesController.validateFullAttributeDeletionProcess(ownSharedAttribute);
        if (validationResult.isError()) {
            return Result.fail(validationResult.error);
        }

        await this.attributesController.executeFullAttributeDeletionProcess(ownSharedAttribute);

+        const canSendMessageResult = await this.messageController.validateMessageRecipients([ownSharedAttribute.shareInfo.peer]);
+
+        if (canSendMessageResult) {
+            return Result.ok({ notificationId: "" });
+        }


I'm so confused.

why do you return Result.ok({ notificationId: "" })? This doesn't make any sense.

if (canSendMessageResult) should always return true. I think you wanted to run if (canSendMessageResult.isError) here

Why do you run two checks, one for Pending and the other validateMessageRecipients check. IMO these checks should do exactly the same.

Please say if I am wrong @sebbi08, but regarding point 2 I think the variable canSendMessageResult should be renamed to messageRecipientsValidationError. Then the implementation should be more clear.

Regarding point 3, the differentiation is necessary in order to allow to delete Attributes even though no Notification can be sent (this is necessary, for example, for "DeletionProposed" Relationships). So we call validateMessageRecipients in order to avoid that an attempt is made to send a Notification which can't be sent, and we check for a pending Relationship before to avoid deleting the Attribute in the first place. (#398 (comment))

packages/transport/src/modules/messages/MessageController.ts

packages/runtime/src/useCases/common/RuntimeErrors.ts

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts

packages/transport/src/modules/messages/MessageController.ts

Co-authored-by: Julian König <33655937+jkoenig134@users.noreply.github.com>

Milena-Czierlinski

Some beauty comments :)

Milena-Czierlinski · 2025-02-07T13:48:03Z

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

        const validationResult = await this.attributesController.validateFullAttributeDeletionProcess(peerSharedAttribute);
+


Suggested change

Milena-Czierlinski · 2025-02-07T13:49:53Z

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

+        const messageRecipientsValidationResult = await this.messageController.validateMessageRecipients([peerSharedAttribute.shareInfo.peer]);
+
+        if (messageRecipientsValidationResult.isError) {
+            return Result.ok({});
+        }


Suggested change

const messageRecipientsValidationResult = await this.messageController.validateMessageRecipients([peerSharedAttribute.shareInfo.peer]);

if (messageRecipientsValidationResult.isError) {

return Result.ok({});

}

const messageRecipientsValidationResult = await this.messageController.validateMessageRecipients([peerSharedAttribute.shareInfo.peer]);

if (messageRecipientsValidationResult.isError) {

return Result.ok({});

}

Grouping results and throwing their errors enhances the readability of the code in my opinion. Please adjust it equally in the other files.

Milena-Czierlinski · 2025-02-07T13:50:02Z

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

@@ -39,13 +41,26 @@ export class DeletePeerSharedAttributeAndNotifyOwnerUseCase extends UseCase<Dele
            return Result.fail(RuntimeErrors.attributes.isNotPeerSharedAttribute(peerSharedAttributeId));
        }

+        const relationshipToPeer = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);
+


Suggested change

Milena-Czierlinski · 2025-02-07T13:51:15Z

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts

@@ -39,13 +41,26 @@ export class DeletePeerSharedAttributeAndNotifyOwnerUseCase extends UseCase<Dele
            return Result.fail(RuntimeErrors.attributes.isNotPeerSharedAttribute(peerSharedAttributeId));
        }

+        const relationshipToPeer = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);


Suggested change

const relationshipToPeer = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);

const relationshipWithStatusPending = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);

Same in other files.

Milena-Czierlinski · 2025-02-07T13:52:21Z

packages/runtime/test/lib/testUtils.ts

@@ -915,3 +917,38 @@ export async function cleanupAttributes(...services: TestRuntimeServices[]): Pro
        })
    );
 }
+
+export async function createRelationshipInPendingState(


Suggested change

export async function createRelationshipInPendingState(

export async function createRelationshipWithStatusPending(

Milena-Czierlinski · 2025-02-07T13:54:47Z

packages/runtime/test/consumption/attributes.test.ts

+                }
+            };
+
+            const content: RelationshipTemplateContentJSON = {


Suggested change

const content: RelationshipTemplateContentJSON = {

const relationshipTemplateContent: RelationshipTemplateContentJSON = {

Milena-Czierlinski · 2025-02-07T13:56:37Z

packages/runtime/test/consumption/attributes.test.ts

+            const item: ReadAttributeRequestItemJSON = {
+                "@type": "ReadAttributeRequestItem",
+                mustBeAccepted: true,
+                query: {
+                    "@type": "IdentityAttributeQuery",
+                    valueType: "GivenName"
+                }
+            };
+
+            const content: RelationshipTemplateContentJSON = {
+                "@type": "RelationshipTemplateContent",
+                title: "aTitle",
+                onNewRelationship: {
+                    items: [item],
+                    "@type": "Request"
+                }
+            };


I'd move this below the declaration of repositoryAttribute, such that is is closer to its usage.

Milena-Czierlinski · 2025-02-07T13:57:39Z

packages/runtime/test/consumption/attributes.test.ts

+            const attributeDeletion = await services2.consumption.attributes.deleteOwnSharedAttributeAndNotifyPeer({ attributeId: ownSharedAttribute.value[0].id });
+
+            expect(attributeDeletion).toBeAnError(


Suggested change

const attributeDeletion = await services2.consumption.attributes.deleteOwnSharedAttributeAndNotifyPeer({ attributeId: ownSharedAttribute.value[0].id });

expect(attributeDeletion).toBeAnError(

const result = await services2.consumption.attributes.deleteOwnSharedAttributeAndNotifyPeer({ attributeId: ownSharedAttribute.value[0].id });

expect(result).toBeAnError(

or attributeDeletionResult.

Milena-Czierlinski · 2025-02-07T13:59:17Z

packages/runtime/test/consumption/attributes.test.ts

@@ -2665,6 +2671,61 @@ describe("DeleteAttributeUseCases", () => {
            expect(updatedPredecessor.deletionInfo?.deletionStatus).toStrictEqual(LocalAttributeDeletionStatus.DeletedByOwner);
            expect(CoreDate.from(updatedPredecessor.deletionInfo!.deletionDate).isBetween(timeBeforeUpdate, timeAfterUpdate.add(1))).toBe(true);
        });
+
+        test("should throw an error trying to delete an own shared Attribute when the Relationship is in status Pending", async () => {


Please check if the comments on this test are also applicable to the other tests you added.

Milena-Czierlinski · 2025-02-07T14:00:59Z

packages/transport/src/modules/messages/MessageController.ts

@@ -419,7 +419,7 @@ export class MessageController extends TransportController {
        return message;
    }

-    private async validateMessageRecipients(recipients: CoreAddress[]) {
+    public async validateMessageRecipients(recipients: CoreAddress[]): Promise<Result<void, CoreError>> {


Suggested change

public async validateMessageRecipients(recipients: CoreAddress[]): Promise<Result<void, CoreError>> {

public async validateMessageRecipients(recipients: CoreAddress[]): Promise<Result<void>> {

I think the Result already implies that it might be an error.

feat: revoke relationship in status pending when the neded attribute …

4473bf6

…is deleted

sebbi08 added the wip Work in Progress (blocks mergify from auto update the branch) label Jan 27, 2025

sebbi08 added 2 commits January 27, 2025 09:42

chore: remove focused test

4f8d26e

Merge branch 'main' into feature/ABL-354

ef7653b

feat: add error message when deleting an attribute while a relationsh…

5e2237a

…ip is still in Pending state

sebbi08 changed the title ~~Revoke relationship in status pending when the neded Attribute is deleted~~ Return an error when deleting a SharedAttribute where the Relationship is still pending Jan 31, 2025

sebbi08 marked this pull request as ready for review January 31, 2025 10:42

sebbi08 added bug Something isn't working and removed wip Work in Progress (blocks mergify from auto update the branch) labels Jan 31, 2025

Merge branch 'main' into feature/ABL-354

d76fbce

sebbi08 requested review from Milena-Czierlinski, Magnus-Kuhn and britsta January 31, 2025 10:44

jkoenig134 reviewed Jan 31, 2025

View reviewed changes

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts Outdated Show resolved Hide resolved

jkoenig134 reviewed Jan 31, 2025

View reviewed changes

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved

jkoenig134 reviewed Jan 31, 2025

View reviewed changes

packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved

revert: unwanted change

77cb61f

britsta reviewed Jan 31, 2025

View reviewed changes

jkoenig134 added enhancement New feature or request bug Something isn't working and removed bug Something isn't working enhancement New feature or request labels Jan 31, 2025

jkoenig134 changed the title ~~Return an error when deleting a SharedAttribute where the Relationship is still pending~~ Deleting a SharedAttribute does not validate if it's possible to send the Notification Jan 31, 2025

chore: pr comments

4e6814f

britsta reviewed Jan 31, 2025

View reviewed changes

packages/transport/src/modules/relationships/RelationshipsController.ts Outdated Show resolved Hide resolved

Milena-Czierlinski reviewed Jan 31, 2025

View reviewed changes

packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved

packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved

packages/transport/src/modules/relationships/RelationshipsController.ts Outdated Show resolved Hide resolved

mergify bot and others added 2 commits February 3, 2025 19:55

Merge branch 'main' into feature/ABL-354

c9f4062

chore: pr comments

f670e7e

britsta reviewed Feb 4, 2025

View reviewed changes

packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved

packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved

Update packages/runtime/test/consumption/attributes.test.ts

cd7fcdc

Co-authored-by: Britta Stallknecht <146106656+britsta@users.noreply.github.com>

Milena-Czierlinski reviewed Feb 4, 2025

View reviewed changes

packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved

packages/runtime/test/lib/testUtils.ts Outdated Show resolved Hide resolved

sebbi08 and others added 5 commits February 4, 2025 10:52

Update packages/runtime/test/lib/testUtils.ts

af5fc20

Co-authored-by: Milena Czierlinski <146972016+Milena-Czierlinski@users.noreply.github.com>

Update packages/runtime/test/consumption/attributes.test.ts

11f0270

Co-authored-by: Milena Czierlinski <146972016+Milena-Czierlinski@users.noreply.github.com>

chore: fix renaming

3724471

chore: fix renaming

e76a5f1

chore: fix sending messsage when relationsip is not activatable anymore

e470075

sebbi08 requested review from jkoenig134, britsta and Milena-Czierlinski February 4, 2025 12:45

jkoenig134 reviewed Feb 4, 2025

View reviewed changes

packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved

mergify bot and others added 2 commits February 4, 2025 13:19

Merge branch 'main' into feature/ABL-354

356abea

chore: improve return type and variable naming

96d653a

britsta reviewed Feb 4, 2025

View reviewed changes

packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved

britsta reviewed Feb 4, 2025

View reviewed changes

packages/runtime/src/useCases/common/RuntimeErrors.ts Outdated Show resolved Hide resolved

sebbi08 and others added 3 commits February 5, 2025 08:17

chore: pr comments

054e1d2

Merge branch 'main' into feature/ABL-354

02f5178

Merge branch 'main' into feature/ABL-354

d9a8294

sebbi08 requested a review from jkoenig134 February 6, 2025 09:07

jkoenig134 reviewed Feb 6, 2025

View reviewed changes

packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts Outdated Show resolved Hide resolved

jkoenig134 reviewed Feb 6, 2025

View reviewed changes

packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved

sebbi08 and others added 4 commits February 6, 2025 11:29

Update packages/transport/src/modules/messages/MessageController.ts

d8dce26

Co-authored-by: Julian König <33655937+jkoenig134@users.noreply.github.com>

chore: make notificationId optional

4eb668e

chore: make notificationId optional

a26e5fd

chore: fix tests

03aaebd

Milena-Czierlinski reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

sebbi08 commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

jkoenig134 commented Jan 28, 2025

sebbi08 commented Jan 31, 2025

jkoenig134 commented Jan 31, 2025

jkoenig134 Feb 4, 2025

britsta Feb 4, 2025 •

edited

Loading

Milena-Czierlinski left a comment

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

Milena-Czierlinski Feb 7, 2025

		const validationResult = await this.attributesController.validateFullAttributeDeletionProcess(peerSharedAttribute);

	const relationshipToPeer = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);
	const relationshipWithStatusPending = await this.relationshipsController.getRelationshipToIdentity(peerSharedAttribute.shareInfo.peer, RelationshipStatus.Pending);

	export async function createRelationshipInPendingState(
	export async function createRelationshipWithStatusPending(

	const content: RelationshipTemplateContentJSON = {
	const relationshipTemplateContent: RelationshipTemplateContentJSON = {

		const attributeDeletion = await services2.consumption.attributes.deleteOwnSharedAttributeAndNotifyPeer({ attributeId: ownSharedAttribute.value[0].id });

		expect(attributeDeletion).toBeAnError(

	public async validateMessageRecipients(recipients: CoreAddress[]): Promise<Result<void, CoreError>> {
	public async validateMessageRecipients(recipients: CoreAddress[]): Promise<Result<void>> {

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

Are you sure you want to change the base?

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

Conversation

sebbi08 commented Jan 27, 2025 • edited Loading

Readiness checklist

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

jkoenig134 commented Jan 28, 2025

sebbi08 commented Jan 31, 2025

jkoenig134 commented Jan 31, 2025

Choose a reason for hiding this comment

britsta Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Milena-Czierlinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebbi08 commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

britsta Feb 4, 2025 •

edited

Loading