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

Mark messages as received-by-gateway so they are not included in the next response #142

Closed
estellecomment opened this issue Apr 22, 2017 · 20 comments · Fixed by #166
Closed

Comments

@estellecomment
Copy link
Contributor

estellecomment commented Apr 22, 2017

From medic/cht-core#3073 :
Modify medic-gateway to call medic-api and change the status of received messages to received-by-gateway after they have been successfully written to the database. This will stop them being sent again next query and reduce the bandwidth requirements.

This will avoid step 4 in the current protocol

To do it, have gateway set the needs forwarding flag to true when storing a new message in DB.

@alxndrsn
Copy link
Contributor

@estellecomment It look like you've done most of the work for this. Is it in progress, or do you want me to finish it off?

@alxndrsn alxndrsn self-assigned this Apr 28, 2017
@estellecomment
Copy link
Contributor Author

I haven't given up on it yet :)
I unassigned myself because I'm doing something else for now, and I don't know how much of a hurry you're in. If it's urgent you can finish it, otherwise I will today/tomorrow.

@estellecomment
Copy link
Contributor Author

@alxndrsn Can you review?

@alxndrsn
Copy link
Contributor

alxndrsn commented May 6, 2017

PR test failures are things like this:

medic.gateway.alert.SettingsDialogActivityTest > generic_shouldDisplayCorrectFields[test(AVD) - 5.0.2] FAILED 

	java.lang.RuntimeException: Waited for the root of the view hierarchy to have window focus and not be requesting layout for over 10 seconds. If you specified a non default root matcher, it may be picking a root that never takes focus. Otherwise, something is seriously wrong. Selected Root:

The espresso tests intermittently fail on Travis with errors like this. I'll re-run and see if they pass the second time.

estellecomment referenced this issue May 17, 2017
In current protocol, steps 2 and 4 pass on the same WO message to gateway, which is useless : https://github.com/medic/medic-docs/blob/e4483cbae3b619f1db8f1990f43cd25905e7b09d/user/message-states.md

This change does make sure that WO message receipt by gateway is passed back to webapp, so that webapp doesn't send twice.

See the medic-docs change for clarity : medic/medic-docs@a7be4b0
@alxndrsn alxndrsn self-assigned this May 17, 2017
@alxndrsn alxndrsn reopened this May 17, 2017
@alxndrsn alxndrsn assigned estellecomment and unassigned alxndrsn May 22, 2017
@garethbowen
Copy link
Member

@alxndrsn What's the status of this? Did it fail AT?

@estellecomment
Copy link
Contributor Author

nah, I merged it but @alxndrsn hadn't reviewed it so reverted it.
It got split into two PRs :

I have higher priority stuff that keeps happening, but it's still on my list.

@garethbowen
Copy link
Member

@alxndrsn Would you mind finishing this off while @estellecomment is away?

alxndrsn referenced this issue Aug 3, 2017
Issue: medic/medic-webapp#3408
@alxndrsn
Copy link
Contributor

alxndrsn commented Aug 3, 2017

I didn't have time to refamiliarise myself with the original ticket, and the information about motivation here seems sparse. I would like to fully consider whether this is useful, in line with discussions about the protocol and recent work on the server-side by @SCdF. But if we do want to achieve this, PR at #113

@garethbowen garethbowen added this to To do in 3.11.0 via automation Mar 16, 2020
@garethbowen garethbowen added this to To do in 3.10.0 via automation Mar 16, 2020
@garethbowen garethbowen removed this from To do in 3.11.0 Mar 16, 2020
@garethbowen garethbowen added this to To do in 3.11.0 via automation Apr 28, 2020
@garethbowen garethbowen removed this from To do in 3.10.0 Apr 28, 2020
@garethbowen
Copy link
Member

Delaying as this is a performance improvement only.

@latin-panda latin-panda self-assigned this Aug 5, 2020
@latin-panda latin-panda moved this from To do to In progress in 3.11.0 Aug 5, 2020
@latin-panda
Copy link
Contributor

latin-panda commented Aug 6, 2020

Moving to AT, PR: #160
This branch is based on the branch that solves issue: #139
Please review and get #139 merged first before AT this.

@latin-panda latin-panda moved this from In progress to Ready for AT in 3.11.0 Aug 6, 2020
@latin-panda latin-panda linked a pull request Aug 6, 2020 that will close this issue
@MaxDiz MaxDiz removed this from Scheduled: Assigned to a release in CHT Product Roadmap: Initiatives 2021 Aug 11, 2020
@ngaruko ngaruko self-assigned this Sep 16, 2020
@latin-panda
Copy link
Contributor

Summary

The issue is when API send to Gateway a message (sms), this message was being saved as "not needing forwarding", so its status wasn't updated as "received by gateway" and API might try to send it again to Gateway.

The fix is:
The SMS status should be set as Needs Forwarding when updating sms statuses or when saving new SMS to Gateway DB. Then Gateway will notify the status change to API from the beginning, reducing the chance of API to resend the same SMS to Gateway.

It's important to note that API <-> Gateway communicates in intervals of time, every 2 or 5 minutes.

@newtewt newtewt self-assigned this Sep 25, 2020
@newtewt newtewt moved this from Ready for AT to AT in progress in 3.11.0 Sep 25, 2020
@newtewt
Copy link
Contributor

newtewt commented Sep 28, 2020

@latin-panda I think this is ready to go.

I've pulled the sqlite database and see the message is marked with needs_forwarding of 1. Once the attempt to send and it eventually fails. The needs_forwarding is marked as 0. Looking at the status_history on the message in couchdb. I see the proper flow through. As shown below. Also, have ensured on my phone with service that the messages are being sent properly.

      "state_history": [
        {
          "state": "pending",
          "timestamp": "2020-09-28T14:28:06.231Z"
        },
        {
          "state": "forwarded-to-gateway",
          "timestamp": "2020-09-28T14:28:58.170Z"
        },
        {
          "state": "received-by-gateway",
          "timestamp": "2020-09-28T14:29:58.089Z"
        },
        {
          "state": "forwarded-by-gateway",
          "timestamp": "2020-09-28T14:29:58.089Z"
        },
        {
          "state": "failed",
          "state_details": {
            "reason": "generic; errorCode=-1"
          },
          "timestamp": "2020-09-28T14:29:58.089Z"
        }

@newtewt newtewt moved this from AT in progress to Ready to merge in 3.11.0 Sep 28, 2020
latin-panda added a commit that referenced this issue Oct 8, 2020
Ticket: #142
* This commit will refactor to also notify API of any status change in the sms.
latin-panda added a commit that referenced this issue Oct 8, 2020
Ticket: #142
This commit will update needForwarded flag to send messages statuses to API
@latin-panda latin-panda linked a pull request Oct 8, 2020 that will close this issue
@latin-panda latin-panda moved this from Ready to merge to Done in 3.11.0 Oct 8, 2020
@garethbowen
Copy link
Member

@latin-panda Can this issue be closed now?

@latin-panda
Copy link
Contributor

Hi @garethbowen, yes, it's in "done" status and code was merged :)

Implementing Partners Backlog automation moved this from Scheduled to Done Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sms Type: Improvement Make something better
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

10 participants