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

Add conversations API #8832

Merged
merged 9 commits into from Oct 7, 2018

Conversation

@Gargron
Copy link
Member

commented Sep 29, 2018

Fix #3255

Replacement for computationally-heavy and unsatisfying direct timelines API. The new API would list DM conversations, rather than individual DMs, which allows for UX that people are more used to.

  • REST API: GET /api/v1/conversations
  • Streaming API: New event conversation in the direct stream

  • Data structures
  • REST API
  • Streaming API
  • Tests
  • Basic support in Web UI
  • Polished design in Web UI
  • Keyboard navigation in Web UI

@Gargron Gargron force-pushed the feature-conversations-api branch 3 times, most recently from 51fe688 to 946008c Oct 2, 2018

@ThibG ThibG self-requested a review Oct 3, 2018

@ThibG
Copy link
Collaborator

left a comment

So, I'm not completely sure what conversations are supposed to be here. I gather that they are threads of direct messages and are associated with a given number of participants. Multiple conversations with the same set of participants seem possible. Is that on purpose? It seems like a toot can be part of multiple conversations (through adding/removing participants). Is that also on purpose?

Finally, I'm very confused at how participants are computed, there might be a bug, there? To me, participants would be the author of the toot + all mentioned users. It seems that in this case, it's all mentioned users and not the author of the toot? I think there might be a bug there.

(Haven't reviewed front-end or streaming API changes)

end

def add_status(recipient, status)
conversation = find_or_initialize_by(account: recipient, conversation_id: status.conversation_id, participant_account_ids: participants_from_status(status))

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 3, 2018

Collaborator

It's somewhat unlikely to happen, but I'm worried about race conditions here.

private

def participants_from_status(status)
(status.mentions.pluck(:account_id) - [status.account_id]).sort

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 3, 2018

Collaborator

The toot's author isn't part of the conversation… ?

while (last_status_id = conversation.status_ids.pop)
last_status = Status.find(last_status_id)
break if last_status
end

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 3, 2018

Collaborator

I find this loop slightly confusing. Maybe a comment should be added to make clear that it only removes “orphaned” items? Also, why not remove “orphaned” items regardless of whether the last toot was removed?

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

not sure we even need a loop here? If you're just removing a single status at a time, there's no reason for any other status to be in conversation.status_ids but not actually exist in the database, right?

This whole thing would be less confusing with a normal rails has_many relationship

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 5, 2018

Collaborator

Because it only removes toots from the array if the removed toot is the last one. This is to track the last status, which should always be the visible one.

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 5, 2018

Author Member

Indeed, we don't update the record if a deleted status is not the last status, therefore the array could contain IDs of statuses that no longer exist.

@Gargron Gargron force-pushed the feature-conversations-api branch from 946008c to 869c7bd Oct 3, 2018

end

def add_status(recipient, status)
conversation = find_or_initialize_by(account: recipient, conversation_id: status.conversation_id, participant_account_ids: participants_from_status(recipient, status))

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 3, 2018

Collaborator

(Re-stating my concern about race conditions as github marked my comment as obsolete)

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

Not sure I understand. Shouldn't find_or_initialize_by be atomic?

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 5, 2018

Collaborator

find_or_createis not atomic: https://apidock.com/rails/v4.0.2/ActiveRecord/Relation/find_or_create_by
But here, it's even worse, we are using find_or_initialize_by, which means the database record is only created when we call save a few lines further.
There is a possibility multiple incoming toots for a same new conversations would be processed in parallel. This could for instance occur if sidekiq is down while someone sends us a string of DMs, which could then be processed at the same time when sidekiq goes up, resulting in two conversations for the same threads.

@Gargron Gargron force-pushed the feature-conversations-api branch from 6c118e7 to a1b16cc Oct 3, 2018

# last_status_id :bigint(8)
#

class ConversationAccount < ApplicationRecord

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

I think AccountConversations makes at least a little bit more sense here.

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 5, 2018

Author Member

You're right, that makes perfect sense, and I hate how much I'm gonna need to search and replace here oof

@nightpool
Copy link
Collaborator

left a comment

Incomplete, sorry :(

end

def pagination_params(core_params)
params.slice(:limit).permit(:limit).merge(core_params)

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

I don't think we need both slice and permit here? Just permit should be nearly identical

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 5, 2018

Author Member

Slice is required to avoid warnings in the log about unpermitted parameters being passed.

set_pagination_headers(next_path, prev_path)
end

def next_path

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

is all this pagination boilerplate really necessary? can't it be abstracted into a module?

this._selectChild(elementIndex);
}

_selectChild (index) {

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

all the other methods in this class are shorthand lambda types, so this one probably should be as well? Or vice-versa?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2018

Author Member

Because this method is called from other methods rather being an event handler directly, there are no issues with this binding that the other syntax is meant to solve.

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2018

Collaborator

yeah, the lambda form is probably better but we should be consistent throughout the file (and ideally throughout the project), lexical vs function target this is a hard distinction to remember in the moment.

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2018

Author Member

Event handlers use =, everything else uses the normal class method form.

def push_to_streaming_api
return unless subscribed_to_timeline?

if destroyed?

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

unless destroyed?, remove the comment

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 5, 2018

Author Member

It's a stub. There probably should be a delete event for these. The problem is event naming. It would have to be called something ugly like "conversation_delete"

@@ -69,7 +69,7 @@ const expandNormalizedNotifications = (state, notifications, next) => {
}

if (!next) {
mutable.set('hasMore', true);
mutable.set('hasMore', false);

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

unrelated?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2018

Author Member

It's a bug I found when copying the code to conversations reducer. I think it's relevant.

belongs_to :last_status, class_name: 'Status'

def participant_account_ids=(arr)
self[:participant_account_ids] = arr.sort

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

does find_or_initialize_by bypass this?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 5, 2018

Author Member

No, it doesn't

end

def add_status(recipient, status)
conversation = find_or_initialize_by(account: recipient, conversation_id: status.conversation_id, participant_account_ids: participants_from_status(recipient, status))

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

Not sure I understand. Shouldn't find_or_initialize_by be atomic?

while (last_status_id = conversation.status_ids.pop)
last_status = Status.find(last_status_id)
break if last_status
end

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

not sure we even need a loop here? If you're just removing a single status at a time, there's no reason for any other status to be in conversation.status_ids but not actually exist in the database, right?

This whole thing would be less confusing with a normal rails has_many relationship

# account_id :bigint(8)
# conversation_id :bigint(8)
# participant_account_ids :bigint(8) default([]), not null, is an Array
# status_ids :bigint(8) default([]), not null, is an Array

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

Not sure I understand why we're using postgres arrays here instead of two has_many tables. It makes the model code somewhat brittle and hard to understand

This comment has been minimized.

Copy link
@ThibG

ThibG Oct 5, 2018

Collaborator

Not sure about participant_account_ids, but afaik, status_ids makes sense because it's ordered and only used to track the latest status in a thread more efficiently when deleting statuses.
(Although it probably doesn't handle toots received out-of-order that well, hm)

# updated_at :datetime not null
# severity :integer default("silence")
# reject_media :boolean default(FALSE), not null
# reject_reports :boolean default(FALSE), not null

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 5, 2018

Collaborator

unrelated to this pull?

This comment has been minimized.

@Gargron Gargron force-pushed the feature-conversations-api branch from 2b045e3 to 74b20c8 Oct 5, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2018

Do conversations with blocked users disappear from the list? It doesn't seem so by having a quick look at the code.

@Gargron Gargron force-pushed the feature-conversations-api branch from 5556e3a to 8cb3f84 Oct 7, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

I'm still worried about possible race conditions, but otherwise, this looks fine.
I will probably try that next week on my instance.
What about existing conversations (prior to the migration), though?

@rhaamo

This comment has been minimized.

Copy link

commented Oct 7, 2018

That's really great !

I only have one concern about the fact that blocking or muting an user would remove the actual conversations : in a context of harrassment it could be useful to not delete thoses conversations but rather kept them, but still having the possibility to delete a whole conversation afterward. (you block the user or mute and then deal with reporting with the actual conversation accessible..)

Also if it stays like that (deleting conv on mute/block) this should be indicated in various block/mute(/mute instance maybe too ?) popups !

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

Hm, yeah, having an API to delete conversations instead of deleting them on block/mute would be nice (sorry about my conflicting earlier proposal). Shouldn't be hard to do either, although I'm not sure how the UI would look like.

EDIT: It would also solve the issue about deleting conversations where some participant has been blocked and some haven't been, since it's not clear what to do in that situation.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2018

I only have one concern about the fact that blocking or muting an user would remove the actual conversations : in a context of harrassment it could be useful to not delete thoses conversations but rather kept them, but still having the possibility to delete a whole conversation afterward. (you block the user or mute and then deal with reporting with the actual conversation accessible..)

Block & mute-with-notifications has always deleted notifications from the user. I think it would be strange if the conversations were not cleaned up the same way. The latest behaviour in the PR is consistent with everything else, I believe.

we could add the rails lock_version column to avoid race conditions

There might be no need, the docs say lock_version only works within a single Ruby process anyway so if anything we would need SELECT ... FOR UPDATE

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2018

EDIT: It would also solve the issue about deleting conversations where some participant has been blocked and some haven't been, since it's not clear what to do in that situation.

The block behaviour hides posts where a blocked person is even just mentioned, so that seems already decided.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2018

This locking mechanism will function inside a single Ruby process. To make it work across all web requests, the recommended approach is to add lock_version as a hidden field to your form.

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2018

Anyway, what I was saying, DM conversations usually don't have so many participants that an update happens within the same second to the point a race condition would actually occur. But if it does, I am ready to add a locking mechanism in the future.

I just want to merge this now

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

As I said, a likely possibility is getting two successive DMs from the same person. Depending on the load, they could very well be processed concurrently.

@@ -525,9 +526,11 @@ const startWorker = (workerId) => {
ws.isAlive = true;
});

let channel;

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2018

Collaborator

Not sure I understand this change. Why make it mutable? We never fall through any of the switch cases, right?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2018

Author Member

A few of the switch cases needed to reuse the same assembled string (channel) in two places at once, but a switch clause is not a separate scope, so you can't redefine a const there. Therefore, it's easiest to define the channel up there.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Oct 7, 2018

LGTM!

@Gargron Gargron merged commit 774ac47 into master Oct 7, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate Approved by Eugen.
Details

@Gargron Gargron deleted the feature-conversations-api branch Oct 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.