Fulfill promise from contact saving

[gsvr]

return self.im.contacts.save(self.contact) in self.store_contact_data returns a promise that is not chained properly.

[gsvr]

@imsickofmaps Please review.

[imsickofmaps]

Is there a test that's missing that would have caught this? Can we add one to ensure it doesn't reoccur...

[gsvr]

I can't think of a way to replicate this for testing. In the testing harness the unreturned promise still seems to be working - the contact data is saved and it reaches the next state.

[hodgestar]

It is going to be very hard to test this because it's essentially a timing issue. The code might have originally been written the way it was intentionally (i.e. to not slow down responding to the USSD menu). However, I think this is nicer.

[hodgestar]

@rudigiesler Thoughts?

[gsvr]

@hodgestar @rudigiesler So you know, I opened this issue because it seems like the most likely cause of an issue I'm experiencing in the clinic-finder application, where the saved contact data is not available when needed in the state following the LocationState. There may be other solutions for that problem if the code is like this intentionally (see #19 , for instance).

[hodgestar]

@gsvr I wonder if the problem you're encountering isn't that you're relying on the old copy of the contact in the new location state? If it's the state immediately after, it might be better to pass the information through in a different way? E.g. creator_opts? Link to the problematic code?

[gsvr]

@hodgestar https://github.com/praekelt/go-usaid-clinicfinder/pull/1/files#diff-bd9c9dcd314f2d7df52935b3a6a4d504R153 - I was hoping that I would see data logged here in stead of {}. In the tests it looks like there is data available on the contact: https://github.com/praekelt/go-usaid-clinicfinder/pull/1/files#diff-d118d017711e90fc081f7548515c5436R396.

I think #19 was aimed at passing the information through via creator_opts.

[hodgestar]

@gsvr Ah, I can see the real issue.

You're passing self.contact into self.locate(...). Here self refers to your app. This is not the same as the self.contact that is updated the LocationState and where self refers to the state.

Your tests pass because they pull the contact from the dummy contact store data where the contact has been successfully store.

I think there is a case to be made for allowing one to pass an optional contact object into the LocationState so it doesn't do its own lookup and updates the contact you already have.

[imsickofmaps]

I'd like us to still land this, the internal call to the contact store is way quicker than other API calls this state makes. The unreturned promise is a dangerous idea IMO. We'll make it the responsibility of the app using it to save the contact before entering this state (if it needs to) and refreshing it after (if it wants to use the values) to avoid complexity for now around passing contacts around. Can we land this as is?

[hodgestar]

:+1: