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

Failed replies may disappear from UI #294

Closed
heartsucker opened this issue Apr 3, 2019 · 3 comments · Fixed by #530 or #578
Closed

Failed replies may disappear from UI #294

heartsucker opened this issue Apr 3, 2019 · 3 comments · Fixed by #530 or #578
Assignees
Labels
bug Something isn't working
Milestone

Comments

@heartsucker
Copy link
Contributor

When a reply is sent to a source, it is added to the conversation without a corresponding database object (i.e., it is transient). A UUID is attached to that widget so when the reply API call is successful or failed the widget can update accordingly.

Consider this:

  1. Source has messages S1, S2, and S3 in the UI
  2. Journalist sends reply (which we would expect to be index J4)
  3. Source sends another message. Depending on how the server receives these, it may be either:
    1. S4 (meaning J4 is really J5)
    2. it is S5 (and J4 is unchanged)
  4. When we update the messages, we call self.clear_conversation() then rebuild the UI from the messages in the database. These are populated from what the server tells us.

This means that if the reply the journalist sent was never received by the server, it will disappear from the UI (bug), or that the UI may re-order so the journalist sees a different order of messages in the UI after the sync (possible bug?).

@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Apr 15, 2019

Note

This issue came up when @heartsucker and I were discussing issue #289, which occurred when we were expecting to have a Reply database object before creating a widget that would be displayed in the UI. The bug was fixed so that we once again no longer expecting for the reply to be saved locally first, see send_reply method:

    def send_reply(self, message: str) -> None:
        msg_uuid = str(uuid4())
        self.conversation.add_reply(msg_uuid, message)
        self.controller.send_reply(self.source.uuid, msg_uuid, message)

This is the reason we could end up in a situation where a journalist loses the reply that they wrote. What we should do instead is make sure the reply is stored locally first.

As far as reordering goes, I personally think it's fine to "reorder." If a journalist is offline, maybe by choice or because of a networking issue, then they should be able to see that their replies have not actually been sent yet (maybe by a queued flag or something). When they go back online, it shouldn't surprise them that that is when their replies are sent and for those replies to show up after any new source messages that came in during the period they were offline.

i. S4 (meaning J4 is really J5)

So, yes, I think this is fine. In this scenario J4 is J4-queued which will become J5 after S4 comes in.

ii. it is S5 (and J4 is unchanged)

This would make sense if J4 was actually sent

@eloquence eloquence added this to the 0.2.0alpha milestone Jul 2, 2019
@redshiftzero redshiftzero changed the title Replies sent may appear out of order because of conversation clearing during sync Failed replies may disappear from UI Jul 19, 2019
@redshiftzero
Copy link
Contributor

UI may re-order so the journalist sees a different order of messages in the UI after the sync

This is the expected behavior, some reordering may be necessary on the client side such that there is consistent ordering between server (what the source sees) and client (what the journalist sees).

if the reply the journalist sent was never received by the server, it will disappear from the UI (bug)

This is a legitimate bug, we should store the failed replies persistently somewhere in the local database in order for them to persist in the conversation view (renamed this ticket accordingly).

@redshiftzero
Copy link
Contributor

Woops this was accidentally closed by #530 when we were saying in the PR description "this does not fix #530" ha - GitHub saw "fix #530" and closed it on merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants