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

Implement reducers and actions for outbox and use it in ComposeBox #977

Merged
merged 1 commit into from Aug 9, 2017

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented Aug 3, 2017

This basically queues up all the messages to be sent in an outbox, and uses local_id to check and remove messages from the queue when the message comes back from the server

@smarx

This comment has been minimized.

smarx commented Aug 3, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

subject: string,
content: string,
) => async (dispatch: Dispatch, getState: GetState) => {
dispatch(sendMessageAction({ type, to, subject, content, localMessageId: new Date().getTime() }));

This comment has been minimized.

@kunall17

kunall17 Aug 3, 2017

Contributor

@timabbott I've used the new Date().getTime() as the local id and avoided using indices as I am using an array here!
This should be fine, right?

This comment has been minimized.

@timabbott

timabbott Aug 3, 2017

Member

What we did in the webapp is to use the last message ID we have + 0.01, since we sort messages by ID in the frontend. But from a server perspective, it can be any float; the main thing that's important is that a given client not give multiple messages the same local_id.

This comment has been minimized.

@kunall17

kunall17 Aug 3, 2017

Contributor

Ok, then it will work fine! 👍

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 4, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

1 similar comment
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 5, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 changed the title from Implement reducers and actions for outbox and use it in ComposeText to Implement reducers and actions for outbox and use it in ComposeBox Aug 5, 2017

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 5, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

1 similar comment
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 7, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 9, 2017

@borisyankov Updated with test's!

@zulipbot zulipbot removed the needs review label Aug 9, 2017

@borisyankov

Almost ready to merge!

@@ -8,10 +10,14 @@ export default async (
to: string | string[],
subject: string,
content: string,
local_id: number,

This comment has been minimized.

@borisyankov

borisyankov Aug 9, 2017

Contributor

Be consistent, either call it localId or rename eventQueueId to queue_id (I'd say the first is better)

@@ -11,11 +11,23 @@ import { registerAppActivity } from '../utils/activity';
import { checkCompatibility } from '../api';
import LoadingScreen from '../start/LoadingScreen';
import CompatibilityScreen from '../start/CompatibilityScreen';
import { Auth, Outbox, Actions } from '../types';
import { attemptToSendMessages } from '../outbox/outboxMessageActions';

This comment has been minimized.

@borisyankov

borisyankov Aug 9, 2017

Contributor

Nit: It is a popular convention to call these functions 'tryDoSomething' so in that case trySendMessages would be the more conventional name (or tryToSendMessages but I think try is enough).

@zulipbot zulipbot added the reviewed label Aug 9, 2017

@zulipbot zulipbot added needs review and removed reviewed labels Aug 9, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 9, 2017

@borisyankov Done!

@borisyankov borisyankov merged commit 583b2ac into zulip:master Aug 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 46.587%
Details

@zulipbot zulipbot removed the needs review label Aug 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment