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

Rk unsw #517

Merged
merged 6 commits into from
Jan 14, 2019
Merged

Rk unsw #517

merged 6 commits into from
Jan 14, 2019

Conversation

atton16
Copy link
Contributor

@atton16 atton16 commented Jan 13, 2019

Hi we have made some changes as follows:

  • Fixed bugs related to mode/purpose confirm mechanics
  • Get back Dashboard tab
  • Disable Trip-end Prompt
  • Display only analyzed trips on Diary tab

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@asiripanich I am not going to provide input on your UX choices because the whole point of the open source platform is that you can experiment with what works for you. I will just provide some feedback on what I have seen other groups do in similar areas:

  • Get back Dashboard tab: This is surprising because most groups did not want to inadvertently influence behavior during the study (the Hawthorne effect?)
  • Disable Trip-end Prompt: yeah, CCI disabled the trip-end prompt too. Miriam (the PI) really, really liked it, but some of the users in their pilot hated it. We talked about having an opt-in option as part of the onboarding, but ran out of time. I would really encourage you to explore an opt-in option during onboarding.
  • Display only analyzed trips on Diary tab: I'd encourage you to be cautious about this. In the bic2cal study, people expected some kind of instant feedback at the end of a trip that their trip had registered. If they didn't see that, they thought that the app was not working. Given the latency in transmitting data + running the pipeline, it might be better to leave draft trips, or at least warn people about some delay in seeing the results (like the old moves app used to).

tripMode = mode;
Logger.log("trip" + JSON.stringify(trip)+ "mode" + JSON.stringify(tripMode));
if (!found) {
tripMode = mode;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some comments around what you are trying to do here? Are you trying to find the first manual/mode_confirm object related to this trip? what happens if there are multiple (e.g. the user selected one entry and then changed their mind and selected another)? You should really find the most recent one.

Also, as outlined in the related issue, processed trips will not have their start_ts and end_ts match the user input values because we extrapolate the start position and time of the trip. On the server, the matching user input objects can be retrieved using get_user_input_for_trip_object in emission/storage/decorations/trip_queries.py

We should discuss how you plan to implement the matching on the client. I don't yet see your changes to pull from the UnifiedDataLoader though...

Copy link
Contributor Author

@atton16 atton16 Jan 14, 2019

Choose a reason for hiding this comment

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

It appears that BEMUserCache.getAllMessages return the latest entry first. As this commit is a bug fix, so we simply inverse the logic. The initial commit selected the last item of the loop and results in incorrect value. What we are doing is exactly as you suggested. We are trying to make the mode/purpose confirm editable and displaying the latest chosen value as button label.

@@ -5,8 +5,7 @@ angular.module('emission.controllers', ['emission.splash.updatecheck',
'emission.splash.pushnotify',
'emission.splash.localnotify',
'emission.survey.launch',
'emission.stats.clientstats',
'emission.incident.posttrip.prompt'])
'emission.stats.clientstats',])
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work syntactically in spite of the trailing comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, trailing comma is syntactically corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would normally do this since it helps avoid potential bugs.

@shankari
Copy link
Contributor

merging now since this appears to be strictly better than the previous version although it has definitely not addressed all issues.

@shankari shankari merged commit 65b463b into e-mission:rk-unsw Jan 14, 2019
@shankari
Copy link
Contributor

@atton16 @asiripanich merged and deployed. should be live in ~ 10 mins

@asiripanich
Copy link
Member

@asiripanich I am not going to provide input on your UX choices because the whole point of the open source platform is that you can experiment with what works for you. I will just provide some feedback on what I have seen other groups do in similar areas:

  • Get back Dashboard tab: This is surprising because most groups did not want to inadvertently influence behavior during the study (the Hawthorne effect?)

We have found some errors from enabling this feature. So we will disable it in the next PR.

  • Disable Trip-end Prompt: yeah, CCI disabled the trip-end prompt too. Miriam (the PI) really, really liked it, but some of the users in their pilot hated it. We talked about having an opt-in option as part of the onboarding, but ran out of time. I would really encourage you to explore an opt-in option during onboarding.

Will consider this option.

  • Display only analyzed trips on Diary tab: I'd encourage you to be cautious about this. In the bic2cal study, people expected some kind of instant feedback at the end of a trip that their trip had registered. If they didn't see that, they thought that the app was not working. Given the latency in transmitting data + running the pipeline, it might be better to leave draft trips, or at least warn people about some delay in seeing the results (like the old moves app used to).

I don't want users to answer the trip-end questions for draft trips as they can be very annoying sometimes (very often when I wait to cross the app registers that my trip has ended.). In the next PR we will take out the trip-end questions from draft trips and bring draft trips back to Diary tab.

@shankari
Copy link
Contributor

shankari commented Jan 14, 2019

We have found some errors from enabling this feature. So we will disable it in the next PR.

@asiripanich what were the errors?

@shankari
Copy link
Contributor

shankari commented Jan 14, 2019

very often when I wait to cross the app registers that my trip has ended.

@asiripanich Is this the reason you are disabling trip-end notifications? Because instead of just disabling them, we could add some simple (configurable) checks before the trip end notification was displayed. I've thought about this for a while, but haven't had a sense of what the checks should be. Thoughts?

Also, if you feel that the processed trips are cleaner, another option is to generate the notifications from the server as a push notification. This will have some latency from the end of the trip, but you will know that there is a processed trip ready for the users to provide input for.

@asiripanich
Copy link
Member

Hi @shankari, sorry for super duper late response to this. I lost track of this thread. Anyway, since I've found it via the email exchange you had with my supervisor here is my response. :p

very often when I wait to cross the app registers that my trip has ended.

@asiripanich Is this the reason you are disabling trip-end notifications? Because instead of just disabling them, we could add some simple (configurable) checks before the trip end notification was displayed. I've thought about this for a while, but haven't had a sense of what the checks should be. Thoughts?

Yes, this is part of the reason we decided to disable the trip-end notifications. I'm not so sure what sort of checks we need to prevent this from happening. On a related matter, how Emission handle trip segmentation, I think there should be a way for user to segment the trip or add more info into the trip. For example, if I go to buy lunch at the cafeteria it is not so obvious where I made the stop but instead it registered the whole trip as me going somewhere and return to where I initially started. Does a solution for this exists in Emission that I may not know of?

@shankari
Copy link
Contributor

I am opening a new issue for trip end notification improvements because I think that Ipsita also had some thoughts around it. The changes would need to be made in the native plugin. They are not very hard, and I am happy to make them if we can come up with a clear specification of what the changes should be, and whether they should apply to android or iOS.

@shankari
Copy link
Contributor

On a related matter, how Emission handle trip segmentation, I think there should be a way for user to segment the trip or add more info into the trip. For example, if I go to buy lunch at the cafeteria it is not so obvious where I made the stop but instead it registered the whole trip as me going somewhere and return to where I initially started. Does a solution for this exists in Emission that I may not know of?

Again, there is a very very preliminary PR that @sunil07t worked on earlier that would allow you to break trips and select the mode for each section of the trip (#308). This was then revised by the team from Paraguay (#469).

I have not tested it and I don't know if it works. Take it with a large grain of salt. If you want to pursue this feature further, I would suggest opening a new issue to track the discussion around it. Step 1 being testing the proposed PRs to see if they actually work.

@shankari
Copy link
Contributor

I am actually opening a new issue for the trip editing as well because when I open #469 I see a reference to an issue from @PatGendre too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants