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.

Sign up for GitHub

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

Jump to bottom

XEP-0184: Message Delivery Receipts #1304

Merged
merged 1 commit into from Nov 13, 2018
Merged

XEP-0184: Message Delivery Receipts #1304

merged 1 commit into from Nov 13, 2018

Conversation

ChaosKid42
Copy link
Contributor

This is an attempt to implement XEP-0184: Message Delivery Receipts.

Hope you like it. Any feedback welcome.

@ChaosKid42 ChaosKid42 force-pushed the message_delivery_receipts branch 3 times, most recently from b91abf0 to 0dcd0da Compare November 4, 2018 19:50
jcbrand
jcbrand requested changes Nov 5, 2018
View reviewed changes
Copy link
Member

@jcbrand jcbrand left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChaosKid42 This is great, thank you!

I have added a few comments and questions.

Would you be willing to also add tests for this?

You can add them in spec/messages.js. The existing tests there should give you a good idea of what you need to do.

For example look here:

converse.js/spec/messages.js

Line 23 in e181aaf

it("can be sent as a correction by clicking the pencil icon",

To run the tests in that spec, you can run make serve and then go to http://localhost:8000/tests/?spec=A%20Chat%20Message in your browser.

src/headless/converse-chatboxes.js Show resolved Hide resolved
src/headless/converse-chatboxes.js Outdated Show resolved Hide resolved
src/headless/converse-muc.js Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@ChaosKid42
Copy link
Contributor Author

ChaosKid42 commented Nov 5, 2018 •
edited

Next I will try to write a test for it...

First test implemented. Gonna do a few more to finish it up...

@ChaosKid42 ChaosKid42 changed the title XEP-0184: Message Delivery Receipts WIP: XEP-0184: Message Delivery Receipts Nov 5, 2018
@ChaosKid42 ChaosKid42 force-pushed the message_delivery_receipts branch 3 times, most recently from 1ed16fb to 32fca5c Compare November 7, 2018 12:26
@dwd
Copy link

dwd commented Nov 7, 2018

I'd just like to say this code is exactly what I was after... Looking forward to seeing it in master.

@ChaosKid42 ChaosKid42 force-pushed the message_delivery_receipts branch 2 times, most recently from 9b0b700 to 35b7b37 Compare November 7, 2018 15:59
@jcbrand
Copy link
Member

jcbrand commented Nov 7, 2018

@ChaosKid42 Looks like there are more tests that are not aware of the <receipt> element which is now in the message stanzas...

@jcbrand jcbrand mentioned this pull request Nov 7, 2018
Closed
@ChaosKid42
Copy link
Contributor Author

@jcbrand These tests work ok on my machine. Strange...:

310 specs, 0 failures, 2 pending specs
Finished in 0 seconds
All tests completed!0
Chromium process was still alive. Sending SIGKILL...

@ChaosKid42 ChaosKid42 force-pushed the message_delivery_receipts branch 2 times, most recently from 44a33ce to 5d6ab7f Compare November 7, 2018 18:48
@ChaosKid42 ChaosKid42 changed the title WIP: XEP-0184: Message Delivery Receipts XEP-0184: Message Delivery Receipts Nov 7, 2018
@ChaosKid42
Copy link
Contributor Author

ChaosKid42 commented Nov 7, 2018 •
edited

@jcbrand I finished my PR. What do you think?
For me make check works ok:

312 specs, 0 failures, 2 pending specs
Finished in 0 seconds
All tests completed!0
Chromium process was still alive. Sending SIGKILL...

@jcbrand
Copy link
Member

jcbrand commented Nov 7, 2018 •
edited

@ChaosKid42 For some reason the tests are much more flaky on Travis and there are some heisenbugs that often fail there but not locally on my own machine (or yours apparently).

I've restarted Travis for now...

@jcbrand
Copy link
Member

jcbrand commented Nov 8, 2018

@ChaosKid42 Looks like you didn't commit the updated build file dist/converse.js?

The tests are run against that file, so this might be the reason why the tests fail on Travis.

One more thing, in the changelog, please use the original ticket number #141 instead of this PR's number and make sure the numbers are ordered numerically.

Thanks!

@ChaosKid42
Copy link
Contributor Author

@jcbrand That was the problem. Thanks!

@ChaosKid42 ChaosKid42 force-pushed the message_delivery_receipts branch 4 times, most recently from 6c06845 to 40cec7a Compare November 12, 2018 18:00
@jcbrand jcbrand merged commit da5ca0b into conversejs:master Nov 13, 2018
@ChaosKid42 ChaosKid42 deleted the message_delivery_receipts branch November 13, 2018 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcbrand jcbrand jcbrand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ChaosKid42 @dwd @jcbrand