-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow adding people back into a group #94
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 🔥
You're going to have to find another way to count membership.
Likely members tangle? I can help with causal ordering etc there if you want
return pull( | ||
ssb.messagesByType({ type: 'group/exclude-member', private: true, live: true }), | ||
ssb.query.read({ query, old: true, live: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 this will LIKELY scan the whole database. With DB1 the way to check if you have a flume-view index supporting you, or if it's falling back to scan whole db is https://www.npmjs.com/package/ssb-query#queryexplain-query
console.log(ssb.query.explain({ query, old: true, live: true }))
I think you should stick with messagesByType
and use http://pull-stream.github.io/#pull-merge
NOTE: you must include a compare function with that otherwise you will eat shit. Just compare on receive time or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you're trying to improve things by reducing the number of pull-streams that are live, then db2 will help with this. It's indexes are a lot more flexible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i checked pull-merge but it said it removes duplicates, so doing it this way seemed easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't want things to break if 2 msgs had the same timestamp
]) | ||
|
||
//const record = keystore.group.get(groupId) | ||
//const inGroup = record && record.excluded === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments need tidyup
}) | ||
} | ||
}) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this an else if
on type to be explicit, catch any future junk
// https://github.com/ssbc/ssb-tribes/issues/79 | ||
const members = addedMembers.filter(addedMember => !excludedMembers.includes(addedMember)) | ||
// calculate if the person has been added more times than they've been removed | ||
// this is kind of basic but works. although lol people can add themselves back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 this crude version won't work sorry D:
The problem is if I add you to a Batts Group at the same time as Staltz adds you, then you've technically been added "twice", even though that addition was idempotent. Then I remove you ... then you've been added twice and removed once... so according to this you're in the group, even though you may have honoured the exclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm ok i based this solution on a discussion we had at some point.
a simple solution could be to check this algorithm also whenever we receive something in the mutateMembers stream? to have the 2 systems match up, avoiding mismatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try the tangle stuff in a separate pr instead #95
@@ -8,7 +8,7 @@ const listen = require('../listen') | |||
// TODO this is... not listen any more | |||
// we may need to rename this | |||
|
|||
test('listen.addMember', t => { | |||
test.skip('listen.addMember', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO (undo skip)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the addMember listener is removed though. so just remove the test too? although we'll see after i've rewritten stuff again
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
Fixes #79