Skip to content

Conversation

@chromy
Copy link
Collaborator

@chromy chromy commented Jan 10, 2026

The previous code did:

const process: Command<RemoveOwner>['process'] = input =>
  pipe(
    input.events,
    RA.last,
    O.filter(isEventOfType('OwnerAdded')),
    O.map(() => constructEvent('OwnerRemoved')(input.command))
  );

Which I think prevents any owner being removed except directly after a (possibly unrelated) owner is added.
I've changed it to consider:

  • Only areas which are not removed.
  • Only OwnerRemoved / OwnerAdded for the specified user/area.

Then check the last such event is OwnerAdded.

It's not clear to me that it is good to be so specific in the process step:

  • We have to iterate through all the events
  • while replicating complex business logic from the read-model

Maybe it's better to just allow people to raise OwnerRemoved multiple times and similar and handle it later.

This also had the side effect of breaking many read-model tests regarding various combinations of owners
with old numbers rejoining with new numbers and then getting removed/added as owners on the new/old
number. I think that means that the logic in the read-model never really worked and just looked like it
worked because the test code was failing to raise the events - but I'm not 100% sure.
I have skipped the broken tests for now.

@Lan2u
Copy link
Collaborator

Lan2u commented Jan 11, 2026

It's not clear to me that it is good to be so specific in the process step:
We have to iterate through all the events
while replicating complex business logic from the read-model
Maybe it's better to just allow people to raise OwnerRemoved multiple times and similar and handle it later.

This is my thought aswell - I think this is a bit of a hold-over from before we had the shared read model. Now that we do that should be used as a 'best-effort' way to check conditions and if those are ok then commit the event and deal with proper handling on the read side

@Lan2u
Copy link
Collaborator

Lan2u commented Jan 11, 2026

This also had the side effect of breaking many read-model tests regarding various combinations of owners
with old numbers rejoining with new numbers and then getting removed/added as owners on the new/old
number. I think that means that the logic in the read-model never really worked and just looked like it
worked because the test code was failing to raise the events - but I'm not 100% sure.
I have skipped the broken tests for now.

Yeah this is again part of the transition towards using the shared model for everything, the various areas that don't use the shared model need updated so they can benefit from the member number linking mechanism

@Lan2u
Copy link
Collaborator

Lan2u commented Jan 11, 2026

I'll merge this for now but we will want to update things to use the shared model (and re-enable the associated tests) before making further changes

@Lan2u Lan2u merged commit ffa2caa into main Jan 11, 2026
6 checks passed
@github-project-automation github-project-automation bot moved this from Options to Done in Makespace Members App Jan 11, 2026
@Lan2u Lan2u deleted the fix-removing-ownership branch January 11, 2026 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants