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

Use result of post request for sending message #1079

Merged
merged 5 commits into from Aug 23, 2017

Conversation

Projects
None yet
3 participants
@kunall17
Contributor

kunall17 commented Aug 22, 2017

Fixes #1077 #1074

@@ -7,6 +7,7 @@ import {
START_OUTBOX_SENDING,
FINISHED_OUTBOX_SENDING,
DELETE_OUTBOX_MESSAGE,
MESSAGE_SEND_SUCCESSFUL,

This comment has been minimized.

@borisyankov

borisyankov Aug 22, 2017

Contributor

MESSAGE_SEND_SUCCESS

@@ -75,6 +75,7 @@ export const CANCEL_EDIT_MESSAGE = 'CANCEL_EDIT_MESSAGE';
export const INIT_ALERT_WORDS = 'INIT_ALERT_WORDS';
export const MESSAGE_SEND = 'MESSAGE_SEND';

This comment has been minimized.

@borisyankov

borisyankov Aug 22, 2017

Contributor

Would that name be MESSAGE_SEND_START ?

This comment has been minimized.

@kunall17

kunall17 Aug 23, 2017

Contributor

*_START seems to be a continuous process that will be done, but here only a single message is added to the outbox

),
);
state.outbox.forEach(async item => {
try {

This comment has been minimized.

@borisyankov

borisyankov Aug 22, 2017

Contributor

Why this change?

This comment has been minimized.

@kunall17

kunall17 Aug 23, 2017

Contributor

Now i'm using the result of POST request, if it is successful then remove the outbox as in the wait period for the EVENT_NEW_MESSAGE with the localMessageId sometimes the outbox triggers again and duplicate messages are sent

);
state.outbox.forEach(async item => {
try {
await sendMessageApi(

This comment has been minimized.

@borisyankov

borisyankov Aug 22, 2017

Contributor

Why added await again? This makes the sending sequential.

This comment has been minimized.

@kunall17

kunall17 Aug 23, 2017

Contributor

Yup, I want that message sending should be completed before calling finish outbox sending.

This comment has been minimized.

@borisyankov

borisyankov Aug 23, 2017

Contributor

But you are waiting on each message before sending the next. You can still send them, wait on them with await Promise.all([

This comment has been minimized.

@kunall17

kunall17 Aug 23, 2017

Contributor

Ohh yeah, will do this another PR as this one is merged!

This comment has been minimized.

@borisyankov

borisyankov Aug 23, 2017

Contributor

I merged this, because I realized your error handling was kinda connected to that.
I would still encourage you to do another PR, but consider how you will handle retries of failed messages too.

kunall17 added some commits Aug 22, 2017

Use result of post request for sending message
And if successful remove the outbox mesasge from state
@@ -33,7 +33,7 @@ export default class Chat extends PureComponent {
handleSend = () => {
if (this.listComponent && this.listComponent.scrollToEnd !== undefined) {
this.listComponent.scrollToEnd();
setTimeout(this.listComponent.scrollToEnd, 300);

This comment has been minimized.

@kunall17

kunall17 Aug 23, 2017

Contributor

This change is because when handleSend is triggered (for scrolling), the renderedMessages in MessageListContainer is not updated yet, hence it was not scrolling to the correct position.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 23, 2017

Looks good (good enough :) )

@borisyankov borisyankov merged commit 00d64e5 into zulip:master Aug 23, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 23, 2017

Hello @kunall17, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 23, 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.

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

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 23, 2017

Hello @kunall17, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

borisyankov added a commit to borisyankov/zulip-mobile that referenced this pull request Aug 23, 2017

Use result of post request for sending message (zulip#1079)
* Use content field as parsed html content

* Use result of post request for sending message

And if successful remove the outbox mesasge from state

* Add self information in the display_recipient array

This fixes the grouping issues!

* Fix flow errors

* Use a delay for scrolling to sent message
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 23, 2017

Hello @kunall17, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

1 similar comment
@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 23, 2017

Hello @kunall17, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

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