Basic member exclusion flow #65
Conversation
|
Updating to the new box2 here #67 |
|
Moving spec checking to a new issue #68 |
|
Made issue about updating members tangle logic #69 |
|
Issue for looping member re-addition #70 |
|
I'm reviewing this now |
There was a problem hiding this comment.
This looks really great. There's a lot of detail here and you're handling it well 👌
The change I'm keen to see is in testing the behavior around Bob post exclusion. Critically - can they read or post to the new group. This is the foundational definition of "excluded" IMO
test/exclude-members.test.js
Outdated
| } = await alice.tribes2.create().catch((err) => { | ||
| console.error('alice failed to create group', err) | ||
| t.fail(err) | ||
| }) | ||
| t.pass('alice created a group') |
There was a problem hiding this comment.
your test says "pass" regardless of if it did, which could confuse people debugging tests. I think the following suggestion is more clear (and less lines)
| } = await alice.tribes2.create().catch((err) => { | |
| console.error('alice failed to create group', err) | |
| t.fail(err) | |
| }) | |
| t.pass('alice created a group') | |
| } = await alice.tribes2.create() | |
| .then(res => t.pass('alice created a group') && res) | |
| .catch(err => t.error(err, 'alice created a group')) |
your fail/ catch case will also cause a bad failure because it will return undefined which will then fail to be destructured 🤷 (don't care about that that much)
There was a problem hiding this comment.
t.error(err, msg)
- if there is no
errprints: ":heavy_check_mark: msg" - if the is an
errprints: ":x: msg"- i can't remember if it logs the error details, but you only really need that when debugging I guess
There was a problem hiding this comment.
this was just copy pasted from some other test. since it's better tested in other places i should maybe just remove it?
There was a problem hiding this comment.
tried the && res thing but didn't work (i guess pass() returns undefined) so just removed the pass lines. cleaned up the console statements though
|
|
||
| t.equal(reinviteMsg.type, 'group/add-member') | ||
| t.deepEqual(reinviteMsg.recps, [groupId, aliceRoot.id]) | ||
| // TODO: check members tangle |
There was a problem hiding this comment.
This is for #69
| const post = secondContents[1] | ||
|
|
||
| t.equal(post.text, 'post', 'found post on second feed') | ||
|
|
There was a problem hiding this comment.
🔥 we need to test more of Bob's experience, specifically:
- confirm he sees the
excludedumping him - confirm he can no longer publish to the group using
ssb.tribes2.publishafter exclude
There was a problem hiding this comment.
I was gonna save that for this #71
There was a problem hiding this comment.
Yeah, I support doing this in a separate PR to avoid "PR constipation".
index.js
Outdated
| publishAndPrune( | ||
| ssb, | ||
| content, | ||
| opts?.feedKeys ?? groupFeed.keys, |
There was a problem hiding this comment.
🌶️ (not a bug but semi-dangerous) the fact that you have getFeed which pivots functionality, then also this line which also has to pivot correctly for things to work makes this feel really brittle to me.
I would request change to any of these
- code duplication (have the two cases clearly seperated)
- a function
getFeedKeyswhich calls back with the keys that are used in this line - a warning comment
| opts?.feedKeys ?? groupFeed.keys, | |
| opts?.feedKeys ?? groupFeed.keys, | |
| // WARNING: this line is tightly coupled to getFeed logic |
There was a problem hiding this comment.
Please make the same change in addMembers
There was a problem hiding this comment.
went with passing the keys through the cb
Co-authored-by: André Staltz <[email protected]>
Co-authored-by: André Staltz <[email protected]>
Co-authored-by: André Staltz <[email protected]>
Starting to implement https://github.com/ssbc/ssb-group-exclusion-spec
clarifyerrorscreatesays "NOTE: Ifcreatefinds an empty (i.e. seemingly unused) group feed, it will start using that feed instead of creating a new one.". I think we might run into that situation the way the newfeedKeysoptions work.create's recovery search for "add-member" messages works