App shows green icon when tracking is inactive

[Krish023]

Linked issues:

Fixes #646

Screenshots:

How to test

[Krish023]

Hi @tstirrat GetStoreData is an async method but it gets called as a sync method. There are two GetStoreData calls inside the checkIfUserAtRisk method. One is setting this.setState({ currentState: StateEnum.NO_CONTACT }) and another setting currentState: StateEnum.SETTING_OFF, . We can't be sure which will set the state first due to the async method. So I have checked inside one to another.

[tstirrat]

Ohhh, good catch regarding the race condition!

Though, even with this change, it's still there. Can you eliminate 🐛 the race condition completely?

Move all of the checking logic inside:

GetStoreData(PARTICIPATE).then(isParticipating => {
  // move all other checks inside here
});

[Krish023]

Ohhh, good catch regarding the race condition!

Though, even with this change, it's still there. Can you eliminate 🐛 the race condition completely?

Move all of the checking logic inside:

GetStoreData(PARTICIPATE).then(isParticipating => {
  // move all other checks inside here
});

Sure. I will change it

[kenpugsley]

Point of style ... can you please rename this PR to something meaningful. It's hard to keep track of which PR is which when we just reference another issue.

[tstirrat]

Point of style ... can you please rename this PR to something meaningful. It's hard to keep track of which PR is which when we just reference another issue.

Yes !!! ^^^

We don't know what issue 123 is so don't make us do more work. We're already very busy

[Krish023]

Point of style ... can you please rename this PR to something meaningful. It's hard to keep track of which PR is which when we just reference another issue.

Yes !!! ^^^

We don't know what issue 123 is so don't make us do more work. We're already very busy

I didn't get the above points. I have already mentioned the issue 646. Do I need to do any changes from my side?

[kenpugsley]

Point of style ... can you please rename this PR to something meaningful. It's hard to keep track of which PR is which when we just reference another issue.

Yes !!! ^^^
We don't know what issue 123 is so don't make us do more work. We're already very busy

I didn't get the above points. I have already mentioned the issue 646. Do I need to do any changes from my side?

Please don't take this the wrong way ... love the work you've done. But it is important that we have people follow the style of how PR's should be done. As Tim said, it's really about helping with the understandability of the PR. If the title of the PR says what it really is, then the reviewer doesn't have to spend time digging out the issue and figuring out what is going on, how to verify, etc. Remember that code reviewers are tracking multiple issues, so issue numbers get hard to remember. It's also much more helpful in the event we need to go back through PR's to figure out what went in, when, and possibly if we need to revert.

Here's an example of what you might have done on this pr (Looking at the discussion, you already ended up writing much of this in your first comment):

For the PR title
Fix LocationTracking race condition that causes incorrect state to be shown (Issue #646)

For the PR Description
GetStoreData is an async method but it gets called as a sync method. There are two GetStoreData calls inside the checkIfUserAtRisk method. One is setting this.setState({ currentState: StateEnum.NO_CONTACT }) and another setting currentState: StateEnum.SETTING_OFF, . We can't be sure which will set the state first due to the async method. So I have checked inside one to another.
... then include the linked issue as you did, plus a quick set of steps to test ...

[tstirrat]

I didn't get the above points.

Oops, I made my comment without good explanation. Ken explained it way better, thanks :)

I think we can/should write a wiki or docs on this to make it clearer 😸

[Krish023]

I didn't get the above points.

Oops, I made my comment without good explanation. Ken explained it way better, thanks :)

I think we can/should write a wiki or docs on this to make it clearer 😸

Noted:)

[tstirrat]

Tested on 2 x android emus and 1x iOS emu. Looks good.