Open tonysurma opened 8 years ago
ping @OhMcGoo
@tonysurma I am looking into this
@tonysurma @seanepping
we could look into using a code-based scheduler for this functionality: http://www.quartz-scheduler.net/
a lot more flexible than kicking off a process from a build in windows scheduler.
PR #1111 starts implementation of this
@tonysurma, what I'd like to add/fix from that PR is communication preferences should not be on the message, but rather stored as user preferences and the lookup for how to communicate to the user will be done via a read the based on preferences, decide how to send.
@tonysurma, can you assign this to me? I'd like to start with the first item, then discuss/investigate how to get some of the other things done.
It's a shame this project is not using NServiceBus, b/c Sagas would be a very good way to coordinate the work with the "web jobs" for the communication of confirming requests.
I might check out some open source schedulers (such as Quartz.net) which might make some of the time-based messages a little easier to create as a code artifact, and not "some job" running in the system somewhere.
@tonysurma, I might not be understanding the "Requestor can specify communication preferences" part of this Issue.
I THINK this means that people who sign up for the site should be able to set their communication privledges in the "Manage Your Account" page.
But after looking at the whiteboard pic uploaded by @OhMcGoo, this looks like it has to do with requestor requests... example, "I want to have a smoke alarm installed in my house".
Either way, users who have signed up with the site should still be able to control their communication preferences...
As far as requestors, they are not registered users, so I'm assuming that communication preference would need to be part of the message they send to our system so we know how to reach out to them...
You are right it is with requesters who are not on our site. That preference would have to come from getasmokealarm so perhaps @OhMcGoo can help bridge that connection.
@tonysurma do we have an Issue for allowing registered users to be able to set their communication preferences? Is so, it would be good to add, even if it goes onto the backlog and is not included in the October/Nov/Dec milestones.
I just took a note to go look as I feel like we did have this but that might be faulty memory.
@tonysurma, I'm reading through PR #1111 as well as the info in this Issue to gain a deeper insight and understanding of what we're trying to do here, and I have a question.
It looks like the existing code for written for handling outside requests into the system first looks up the request in the Request table in the db, then decides whether or not we're creating a request or if we're updating an existing requet's status.
Since this is an inbound integration, why would we ever be getting status updates for existing requests we've created via the same integration from outside our system? I would imagine once the request has been created in our system, our internal workflow will control the status/lifecycle of the request.
Here is the code in question:
var providerId = request?.ProviderId;
var existingRequest = await _dataContext.Requests.FirstOrDefaultAsync(x => x.ProviderId == providerId);
if (existingRequest == null)
{
request.RequestId = Guid.NewGuid();
if (request.Latitude == 0 && request.Longitude == 0)
{
var address = _geocoder.Geocode(request.Address, request.City, request.State, request.Zip, string.Empty).FirstOrDefault();
request.Latitude = address?.Coordinates.Latitude ?? 0;
request.Longitude = address?.Coordinates.Longitude ?? 0;
}
existingRequest = request;
_dataContext.Requests.Add(existingRequest);
}
else
{
existingRequest.Status = request.Status;
}
await _dataContext.SaveChangesAsync();
Thanks
Need to double check with @OhMcGoo and by proxy Cecelia but I believe that a requestor can cancel/'undo'/rescinde a request from their side and that is the only status update that would come from there.
cc @MisterJames who may have talked to them about this
@tonysurma my assumption is the cancel/undo process is via the "Y" and "N" responses to the initial Request. If so, that would be outside the code that accepts creates the initial Request.
Also, I need to know, at least in the context of PR #1111 how the requestor will be attaching their communication preferences to the initial Request so the first line item of this Issue "Requestor can specify communication preferences (text, voice) for communications" can be fulfilled.
@tonysurma @OhMcGoo so forgive me if I'm wrong, but it looks like we don't care about the communication preference when we receive the initial request.
We record the requestor's communication preference when the request is assigned to an itinerary, the initial message goes out to the requestor via MMS, and we get a positive ack back from the requestor in these ways:
(taken from #1032): "When a positive acknowledgement is received, the type of communication should be recorded and become the default for all future correspondence with that requester."
So, if we receive a "no" from the requestor, or nothing at all, we do not record any type of communication preference.
@tonysurma, if a requestor can "cancel" or "undo" a request, we need a way to correlate that request back into our system...
currently, in PR #1111 it looks like we're storing ProviderId, but that's just a string that might say something like "serial" for the Red Cross: https://github.com/HTBox/allReady/blob/0d81313ecf9f65b9f1a5a90d2da3a74c2b56ac84/AllReadyApp/Web-App/AllReady/ViewModels/Requests/RequestViewModel.cs#L18
We'll need a way to correlate back into our system based on the ProviderId as well as a unique identifier for the request for that given provider.
This also means the requestor is listening/recording a response from us, and then storing that unique id somewhere on their end so if and when they need to send an update, we can correlate back to our system.
pinging @OhMcGoo as I really need some info from the get a smokealarm side (pinging @MisterJames as well if he spoke with them.
My understanding was that ProviderId was supposed to be the unique id provided by the other system but I very well may be wrong. We need that both for them to update as well as for us to give them status.
And I think you are correct for now on preferences. Let's do simplest path and we can work to advance that later.
Thanks @mgmccarthy
@mgmccarthy would you be available on Friday for a call? we can sort through this in about 30 mins and then post back our decisions to this thread. I have some updated info from the last few days on the advancement of the API (along with API keys and access to the demo environment) so we should be able to process this quickly.
@MisterJames, I WFH Friday, so it should be pretty easy to fit a call in during the day... let me know what time works for you.
Looks like the RequestId is primary key of the Request table so we already have the ability to correlate each individual based on this value
@MisterJames There may be a reason for it but the request id is a Guid as the moment. Could we perhaps move to int? I'm worried that as we build up 100's of requests searching by Guid is not very efficient. I've not dug through the code yet to see if Guid is really needed. I presume it's the ProviderId which links back to any id's from GetASmokeAlarm.
I'm also keen to know the outcome of the API discussions as I might look to pick up some of the other issues around request status etc.
Guid's can be efficient if you implement sequential guids... aka (CombGuid's). Jimmy Nilsson talks about this in his book "Applying Domain-Driven Design and Patterns: With Examples in C# and .NET" (https://www.amazon.com/Applying-Domain-Driven-Design-Patterns-Examples/dp/0321268202)
We implemented a CombGuid generator at my job b/c all of our PK's are Guid's b/c we're a 100% distributed system and therefore, enforce nothing in the way of PK/FK relationships in our many siloed databases. Id generation is done in the code not in the database.
If we're worried about performance, I could take a look at what we've done and see if we can apply it to AllReady.
What would be easier though, as Steve had mentioned, is if we don't have to use a Guid at all, we could use something like int. An idea would be to use an int for our "internal" id, and then make an ExternalId field that is a string that could store any data type and those two fields together as a composite key so we can support other in-bound integrations that might supply unique id's that are different data types.
@tonysurma, can we get verbiage as to what needs to be on the message for:
my assumption is we ask for confirmation for the message sent a week before and a day before, but the day of message is basically telling the requestor, "hey, we're not coming b/c you never confirmed your request with us"
@tonysurma @MisterJames... in talking shortly with @stevejgordon, it seems that getasmokealarm/Red Cross will be calling the RequestApiController
from PR #1111 so we can take create requests in our system from them.
It seems that getasmokealarm also has the ability to receive an ack that would allow it to track requests it has already sent to allReady. As a result of this, it sounds like getasmokealarm could send us the same request, but with some type of status update to an existing request (even though we're messaging all requestors via sms to confirm or cancel their request).
So, my question is we need to supply getasmokealarm with some type of unique identifier (in the Requst table, it's RequestId) so if they send us a status update for an existing request, we can correlate on our side.
Does this sound correct, or am I mixing up two Issues/Use Cases here?
I also need to know if when providing a "status" update for an existing request, if they send the entire request again (hence, allowing the use of AddOrUpdate), or if they're only going to send the RequestId and the status. Those are two different UC's. I'm assuming the former for now.
Thanks
pinging @tonysurma @MisterJames
I need to know the answers to my questions in the above comment. There are also other things I need to know:
let me know if either of you have time for a quick call/slack chat.... thanks!
@misterjames if you can work on the flow/code items I will work on the content pieces
@tonysurma @OhMcGoo ... quick question about the 7 days before/day before message sent to requestors to confirm their Request...
I'm assuming it's 7 days before/1 day before the Intinerary's date, not the date that the Request was created/added to the Itinerary, correct? I'm making the assumption that requests added to the itinerary will be done on the date of the itinerary, regardless of when the request was created, or when the request was added to the itinerary.
I'm asking b/c if a request is created, then assigned to an Itinerary the day before the Intinerary's date, then we'll need some logic in the code to make sure we're not sending out messages that don't make any sense... like a seven day message.
Yes 7 and 1 day before itinerary date
ping @tonysurma about the question above (do we want to send them a message after they've confirmed "Y")?
Also, a new question. I'm not too sure what the: "On Installer marking previous request in itinerary delivered day of send "See you in a few" message"
line item from the Item description means. I'm assuming it means the day of the request, we send them a message "See you in a few?"
broke out some of line items above into Issue #1405
@tonysurma @MisterJames I stumbled upon Issue #1055 last night which looks like it has some of the information of what to expect from the getsmokealaram requests that our RequestApi will be receiving. I'm not too sure if this is the same requirement for this issue itself, but in reading it, it does look to be so.
For instance, it clearly marks what to expect from the status of each "request" that will be sent to us (new, in progress, installed, or canceled).
It also goes into details about when a request's status changes, to post the new stastus to our /admin/requests/status/:id
endpoint. (whatever that means).
The biggest thing still missing here is how our system is supposed to uniquely identify and incoming request given the current information being sent to us.
Although we track each request internally with a RequestId, the feed from getasmokealarm does not see to have any type of unique identifier on it, but more importantly, will only reply back to the request with a boolean saying whether or not will handle the request. We should be replying back to them with a RequestId, which they could store on their end so we can correlate requests coming int out system...
or even better, receive a unique id from each request they send us, and we can use that id to correlate.
Just trying to pull the information together in one place, but it does seem like Issue #1055 has some of the information I was previously asking questions about/looking for
@tonysurma @MisterJames AND, I just found https://github.com/redcross/smoke-alarm-portal/issues/196, which clearly states:
The serial field can be used as a key back into our database, in case as a future enhancement we want to send updates to AllReady about the request -- e.g. whenever the status changes.
as an example, there is the data being sent to use, and the serial
field looks to be their unique identifier for each request;
serial, // our unique identifier: Red Cross region code - 2-digit fiscal year - integer
if this is the case, and you can confirm with getasmokealarm folks, then I will need to make some changes to the RequestApiController and some of the handlers down the pipeline from commands sent from that controller.
We might want to also think about breaking out API Requests into its own table, and then we can handle the "mapping" from the unique id for the incoming integration into our Request table. Although we don't need to do this now, I can see how having that extra layer of abstraction could allow us to easily integrate other incoming data feeds without impacting our Request table, which is internally how we track requests coming into the system, no matter what the source of the Request is.
Thanks!
@mgmccarthy what are the open q's I can answer. We do wnat to send a confirmation response as you asked but not sure what else is open. Apologies but I think I am getting "issue blind" :)
@tonysurma, I think the problem here is we have bits and pieces of the overall picture of receiving getasmokealaram data, what do with it when we get it, and then the sms confirmation messages spread out over multiple issues...
I'll try to come up with a list of Issues I THINK all talk about the same thing, and get you a list so maybe we can consolidate the bits and pieces of info we have about this process into one "holding" Issue (which points to other issues)
in the meantime...
When I built out the RequestApiController (receiving requests from getasmokealarm)
I need to be clear about the confirmations we're talking about:
It would be a good idea to break out the confirmation message sent back to getasmokealarm into a separate issues (if it's not already a separate issue)
@MisterJames, I need confirmation that the incoming serial
field will indeed be a unique identifier for each request we receive from them. If you can confirm that, then I can get to fixing the code.
Guid vs Id, there's a lot written about that. Not as clear cut as you might think.
@MisterJames: Just doing a quick follow up - have you had chance to look at mgmccarthy's comment?
@EmilyLuijbregts the question I asked @MisterJames above has been answered. As you can see, there was a good bit of confusion and missed information around this issue.
For the latest of what's going on with the getasmokealarm API integration, you can check out https://github.com/redcross/smoke-alarm-portal/issues/196 on the getasmokealarm portal.
that has the most up to date information regarding the integration. Thanks
Thanks @mgmccarthy is there anything @EmilyLuijbregts or I can do to tie this issue to others or help you track the work?
working with @MisterJames on some token generation issues we're having on our side. When that's finished, getasmokealarm will be able to test our endpoint invocation using token-based security.
We still have to figure out how we're going to decide to service a request from getasmokealarm using the region and zip code provided.
@stevejgordon I am working on the call-back to getasmokealarm's api after we receive and accept a request from them, as well as any other subsequent request status updates they would want to receive from us so they can keep their records up to date.
@mgmccarthy If there are any smaller components we can break off into discrete issues (suitable for new contributors) please let me know as we can put them into the plan for next week's code-a-thon. I've not followed along enough to know what's done/not done.
@stevejgordon that might be tough to do with this issue. At this point, I pretty much have the API communication back to getasmokealarm in the works and will be contributing that piece with my next PR.
Let me go through and create some Issues for some outstanding items, and if any of the devs in your codeathon feel up for it, they can pick up the work.
I do know that Issue #1405 is available for anyone to pick up who has Twilio knowledge/experience
@mgmccarthy @stevejgordon anything to update here?
@tonysurma, to sum it up, I just checked off the items that are done and ready for v1 in the original issue list at the top of this issue.
These three items from the list:
All three of these will be completed when Issue #1405 is picked up and coded. Issue #1405 needs to be P1, or we'll have no way for the requestor's response to the SMS to be received by AllReady. The Request can still be updated from Pending Confirmation to Confirmed, but that would have to happen through the UI.
I don't know what this items means:
can you please expand on it? My guess is that the installer finishes the current install. when he fulfills the Request, the next requestor on the Itinerary should receive a text message from the installers saying "see you in a few" ???
Thanks
@mgmccarthy For that last one, yes that's what I understood as well. It's possible we can get even more clever in the future and use the Google API to calculate approx current driving time so an actual ETA can be provided. However for v1 I think a fixed, we're coming style message is a fair target.
It's possible we can get even more clever in the future and use the Google API to calculate approx current driving time so an actual ETA can be provided. :+1:
thanks for the feedback @stevejgordon
@tonysurma, two questions I need answered about two of the line items in the initial comment of this Issue:
right now, I have these sms messages going out on the day of, which means NOT at a specified time (which means they could go out at 12:01 am the day of). The other sms confirmations (seven days before, one day before) go out at 12 noon. Seeing how this message is a message letting the requestor's know not to expect us b/c they never confirmed, I'm thinking these sms messages should go out in the morning. I think a good guess for time would be 8 or 9am?
please see my question about this item in this comment
If you get me a answer to both of these questions, we can mark
On Installer marking previous request in itinerary delivered day of send "See you in a few" message
done as it should be very easy code to complete. Thanks
I'm thinking these sms messages should go out in the morning. I think a good guess for time would be 8 or 9am?
Yes, 9am local time would be proper
On Installer marking previous request in itinerary delivered day of send "See you in a few" message
We don't have this functionality in the mobile app so nothing to implement. When (in the future) the mobile app allows a volunteer to mark (for example) the third request on an itinerary 'complete' the idea is that we would SMS the 4th request to say the equivalent of "We are on the way, See you in a few" so that they know the installer team is coming.
@tonysurma
ok, I will change DayOfConfirmationSmsTextMessage to go out at 9am local tim.
We don't have this functionality in the mobile app so nothing to implement.
but we have the ability to do it through the Web UI (you can mark a Request Completed)... would people in the field not be using the Web UI to do this until the mobile app is ready?
If no, then the
On Installer marking previous request in itinerary delivered day of send "See you in a few" message
can go on the back-burner, although, I did just write code for it yesterday, and it appears to work.
The answer is kinda no, as the web UI can be used at 'anytime' to mark complete like say at the end of the day to mark them all by a coordinator or something.
I will need to think if there is a way to surface this if used during the day one by one via the web UI.
Thanks
@tonysurma Is there discrete tasks that you can break off here for folks in London? We've had a great turnout and folks are looking for issues.