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 action sheet for outbox messages #1022

Merged
merged 13 commits into from Aug 19, 2017

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented Aug 13, 2017

Long press menu for deleting outbox message or retrying to send message

@smarx

This comment has been minimized.

smarx commented Aug 13, 2017

Automated message from Dropbox CLA bot

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

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 14, 2017

The points @Sam1301 made on the previous (already merged) PR are quite valid, and now with this PR I am surer of that...
The more 'container' components we have the more duplication we will have...
We already have a 'Delete' item in the other menu, it just needs enhancing to catch if a message is in the outbox.

And we will not have a 'Retry' menu item. It just have to retry automatically.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 14, 2017

Ok i'll merge them both, but it might be messy and filled with conditional codes!

And we will not have a 'Retry' menu item. It just have to retry automatically.

I think we should have this for now (atleast) as the auto sending might not be perfect and needs some more feedback/improvements
Thoughts @Sam1301 @vishwesh3 ?

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 14, 2017

Then, auto-sending of failed messages will be the highest priority task. The idea of an outbox is 80% about preserving what a user sent but was not received by the backend, and 20% about other stuff like the preview. The main task is not done until it can do that.

@Sam1301

This comment has been minimized.

Member

Sam1301 commented Aug 14, 2017

I haven't looked closely into the outbox functionality/implementation so can't comment on this :).

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 14, 2017

@borisyankov Updated!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 14, 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 14, 2017

Fixes #1032 , @borisyankov LGTM sooner!

@@ -37,6 +37,10 @@ export const isSameRecipient = (message1: Message, message2: Message): boolean =
message1.display_recipient.toLowerCase() === message2.display_recipient.toLowerCase() &&
message1.subject.toLowerCase() === message2.subject.toLowerCase()
);
case 'outbox': {
// If both are an outbox message return true

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

Code is pretty clear without the comment.

handleOutboxMessageLongPress = () => {
const { actions, message } = this.props;
const callback = buttonIndex => {
switch (buttonIndex) {

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

This is not the place we handle the action sheet actions.

}
};
const options = ['Delete message', 'Cancel'];
this.showActionSheet({ options, cancelButtonIndex: options.length - 1, callback });

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

Most of the actions are relevant for an outbox message, why have a specific action sheet for outbox?

const MessageComponent = isBrief ? MessageBrief : MessageFull;
return (
<MessageComponent
message={message}
twentyFourHourTime={twentyFourHourTime}
ownEmail={auth.email}
onLongPress={this.handleLongPress}
starred={this.isStarred(message)}
onLongPress={isOutbox ? this.handleOutboxMessageLongPress : this.handleLongPress}

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

Not here.

realm={auth.realm}
auth={auth}
actions={actions}
handleLinkPress={this.handleLinkPress}
isOutbox={isOutbox}
/>
);
}

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

You need to think more in the direction of 'separation of concerns'.
Adding more and more logic to a single file/component usually breaks it.
MessageContainer ideally should have no idea about the outbox concept.
In fact, outbox should be as similar to non-outbox as possible, so very little 'ifs' are needed.

handleLinkPress?: string => void,
onLongPress: () => void,
isOutbox?: boolean,

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

Can we determine 'isOutbox' later?

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 15, 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 reviewed and removed needs review labels Aug 15, 2017

@Sam1301

Looks good! 👍 Left a comment.

@@ -9,6 +9,7 @@ const reducer = (state: OutboxState = initialState, action: Action): OutboxState
case MESSAGE_SEND: {
return [...state, { ...action.params }];
}
case DELETE_OUTBOX_MESSAGE:

This comment has been minimized.

@Sam1301

Sam1301 Aug 15, 2017

Member

wondering whats the use case here? Is it intended to change the state like the EVENT_NEW_MESSAGE case?
Also might be useful to add a test for this?

This comment has been minimized.

@kunall17

kunall17 Aug 15, 2017

Contributor

When a new message comes in we need to check if the id matches then delete from reducer and similar thing is done in the case of DELETE_OUTBOX_MESSAGE hence we can have the same block!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 15, 2017

@borisyankov Updated!

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

@@ -75,20 +74,20 @@ class MessageFull extends PureComponent {
/>
<View style={styles.contentWrapper}>
<ScrollView style={styles.inner}>
<TouchableWithoutFeedback onLongPress={onLongPress}>
<TouchableOpacity onLongPress={onLongPress}>

This comment has been minimized.

@kunall17

kunall17 Aug 15, 2017

Contributor

I think TouchableOpacity looks better when onLongPress, hence I used this and extracted this to another commit, if you don't agree push without this commit!

This comment has been minimized.

@borisyankov

borisyankov Aug 15, 2017

Contributor

Yeah, I don't think it is good, we had TouchableOpacity before. Most apps I tested had no feedback on long press.

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

@zulipbot

This comment has been minimized.

Member

zulipbot commented Aug 15, 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 17, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 17, 2017

Made the grouping same as normal message, and some enhancements @borisyankov

simulator screen shot 17-aug-2017 9 46 53 am

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

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 18, 2017

Ok, so now i understood what's going wrong

So two events are being received one of MESSAGE_FETCH_SUCCESS and one of EVENT_NEW_MESSAGE

And both of them add a message with the same ID and hence duplicate key's are produced!

Final state is this
So likely it's not a bug with the outbox implementation!

@borisyankov should I check for existing id's while adding in the reducer, or maybe you'll fix this?

Edit: Also I saw on the web app there were only 2 messages in this topic

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 18, 2017

7584c26 prevents the same id messages adding to the state through EVENT_NEW_MESSAGE

Now the problem is the MESSAGE_FETCH_START is being triggered very frequently hence many a times the message which is sent to the server received through MESSAGE_FETCH_SUCCESS and then EVENT_NEW_MESSAGE is not received hence the indicator keeps showing, and the message remains in the outbox!

Not sure if this theory is 100% correct, but seems to be the problem!

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

@@ -93,7 +93,7 @@ export default class MessageHeader extends PureComponent {
if (message.type === 'private' && !isPrivateOrGroupNarrow(narrow) && !isTopicNarrow(narrow)) {
const recipients =
message.display_recipient.length > 1
Array.isArray(message.display_recipient) && message.display_recipient.length > 1

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

Isn't message.display_recipient always an array if type is private?

This comment has been minimized.

@kunall17

kunall17 Aug 19, 2017

Contributor

Nope, if its a private message not a grouped one its a string

@@ -32,7 +32,9 @@ export default class Chat extends PureComponent {
};
handleSend = () => {
if (this.listComponent.scrollToEnd) this.listComponent.scrollToEnd();
if (this.listComponent && this.listComponent.scrollToEnd !== undefined) {

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

When is scrollToEnd undefined?

return { type: 'private', display_recipient: narrow[0].operand, subject: '' };
return {
type: 'private',
display_recipient: narrow[0].operand.split(',').map(item => {

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

That looks like a function to be extracted.

@@ -81,6 +81,23 @@ describe('chatReducers', () => {
});
});
test('appends same id message to state', () => {

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

The description does not match what the test does.

}
: item
}
key={`header${isOutbox ? item.timestamp : item.id}`}

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

Yes, we can have both.

@zulipbot

This comment has been minimized.

Member

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

...state,
messages: Object.keys(state.messages).reduce((msg, key) => {
const isInNarrow = isMessageInNarrow(action.message, JSON.parse(key), action.ownEmail);
msg[key] = isInNarrow ? [...state.messages[key], action.message] : state.messages[key];
const isMessageAlreadySaved = isInNarrow
? state.messages[key].find(item => action.message.id === item.id) !== undefined

This comment has been minimized.

@borisyankov

borisyankov Aug 18, 2017

Contributor

That whole function can be improved, looks a bit unclear.
In fact the best way to refactor it, is to extract the merge code from the MESSAGE_FETCH_SUCCESS and call it with just one parameter.

This comment has been minimized.

@kunall17

kunall17 Aug 19, 2017

Contributor

I refactored the code

EVENT_NEW_MESSAGE is very different from MESSAGE_FETCH_SUCCESS, hence did not take code from there!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 18, 2017

The PR is moving along very well, but there is more work to be done.
Keep in mind it is high-priority! I'll also be tweaking the message list more to improve performance.

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

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 19, 2017

@borisyankov Done with the changes and refactored the EVENT_NEW_MESSAGE chatReducer a bit!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Aug 19, 2017

Cool. Another epic PR is ready to merge.
Keep in mind, more work on the outbox is likely needed (as the message list is not outbox aware anymore and might have broken something)

@borisyankov borisyankov merged commit a14a9d8 into zulip:master Aug 19, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

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

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