Skip to content
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

Remove unregistered group members #1052

Conversation

haffenloher
Copy link
Contributor

First time contributor checklist

Contributor checklist

  • My contribution is fully baked and ready to be merged as is
  • My changes are rebased on the latest master branch
  • My commits are in nice logical chunks
  • I have followed the best practices in my commit messages
  • I have made the choice whether I want the BitHub reward or not by omitting or adding the word FREEBIE in my Git commit messages
  • I have tested my contribution on these platforms:
  • Fedora 25, Chromium 55
  • My changes pass all the local tests 100%
  • I have considered whether my changes need additional tests, and in the case they do, I have written them

Description

Locally remove unregistered 🦆 🐔 🐝 from group membership lists.

Fixes #989
Related to signalapp/Signal-Android#6175

// FREEBIE

});
var members = this.get('members');
members.splice(members.indexOf(source), 1);
this.set({members: members});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to tell the model to just update itself from textsecure.storage.groups instead of doing this? comments appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

textsecure.storage.groups should eventually be dropped so we only have one source of truth for group membership.

If you want to be more concise you can use underscore:

this.set({
    members: _.without(this.get('members'), source)
});

@haffenloher
Copy link
Contributor Author

Looks like I need to test this some more.

@haffenloher haffenloher closed this Feb 6, 2017
@haffenloher
Copy link
Contributor Author

Okay, this works correctly in my tests now. Reopening.

@haffenloher haffenloher reopened this Feb 7, 2017
@@ -221,7 +221,9 @@

if (conversation.get('type') === 'group') {
errors.forEach(function(e) {
if (e.name === 'UnregisteredUserError') {
if (e.name === 'UnregisteredUserError' ||
(e.name === 'HTTPError' && e.code === 404))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps getKeysForNumber() should just throw UnregisteredUserError in this case?

});
var members = this.get('members');
members.splice(members.indexOf(source), 1);
this.set({members: members});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

textsecure.storage.groups should eventually be dropped so we only have one source of truth for group membership.

If you want to be more concise you can use underscore:

this.set({
    members: _.without(this.get('members'), source)
});

} else {
this.saveErrors(result.errors);
errors = result.errors;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a slight change in the order of operations since L215-218 now execute before saveErrors instead of after. Not sure if it matters, but might be best to avoid any unneeded change in behavior.

@haffenloher haffenloher force-pushed the remove_unregistered_group_members branch from ee4e673 to cfa86df Compare February 8, 2017 17:11
@haffenloher
Copy link
Contributor Author

All done, thanks for the feedback!

@haffenloher haffenloher force-pushed the remove_unregistered_group_members branch from cfa86df to 82cd482 Compare February 8, 2017 19:04
@liliakai liliakai closed this in a768b94 Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants