Develop an integrated survey solution for onboarding and potentially each trip #727
Comments
|
As of e-mission/e-mission-phone@0636a90, screenshot e-mission/e-mission-phone#826 (comment) we have the basic survey functionality working again. But there is still some special casing of "SURVEY" in the code that reads the manual inputs. Let's fix that next. |
|
It is very tempting to refactor the trip button code even further to make it beautiful and to generalize to a single input per trip, but we don't have time for that right now. Our focus should be on what the enketo libraries do. We should just comment/no-op everything to see what the survey code actually uses, and generalize that to work for the onboarding survey as well. |
|
Tried to remove the special casing of "SURVEY" by moving the code to |
|
One fix might be to remove the dependency between the directive service and the diary helper. There doesn't appear to be a reason why that matching needs to happen in the directive. Why doesn't it just happen in the service, right after we read the input, or in the controller? |
|
maybe because we need to iterate over the user inputs - e.g. We could get the keys (e.g. Since this is only used for the trip label matching, it seems to make sense to move it to a separate service that is directly in the |
|
Moved the matching code to the newly created because then the value stored in the map is a promise The obvious fix is to listen to the promise and then set the value However, in this case, the result map is filled in asynchronously, which means that it is not properly accounted for in |
|
wait, that can't be the reason because and then The multi-label works, and it also has the manual map as blank at this stage Ah the multilabel works because the After overriding with the labels, we do get some new inputs |
|
Reverting changes and restoring them one-by-one with logs to ensure that everything works. Working version, survey is an array |
Two main reasons: - It really doesn't belong there; the rest of the functions there deal with formatting trips and maps, and are in a common service to avoid duplication between the diary and the common tabs. - It causes a circular dependency when we try to call the directive to process the inputs that we read e-mission/e-mission-docs#727 (comment) e-mission/e-mission-docs#727 (comment) e-mission/e-mission-docs#727 (comment)
We do this as part of the promise response instead of folding it into the `processManualInputs` function because enketo survey answers also returns a promise. So we will have to set the `resultMap` asynchronously, which may run into timing issues e-mission/e-mission-docs#727 (comment) Let's revisit this later once we figure out how the answers should work
Instead, define `MANUAL_KEYS` in each directive and iterate over them. Also, it turned out that this was not working earlier e-mission/e-mission-docs#727 (comment) not because of any timing issues, but because we were reading the wrong key TODO: see if we can combine `extractResult` and `processManualInputs` since the timing was not the issue after all. However, the timing *could* be an issue since it is async, and we don't want to rely on timing effects, so we may also want to leave it unchanged. `
The previous implementation treated the enketo code as another type of survey. However the enketo survey is different than the multi-label UI. We have now separated out the functionality into separate directives, and removed all references to the ConfirmHelper in the diary code The eneketo survey is a single array entry with key "SURVEY", and is stored in the directive. In the diary, deployers can choose between the multi-ui and enketo implementations e-mission/e-mission-docs#727 (comment)
|
Next steps:
Let's now try to understand the enketo wrappers better and pull out the load and save functionality into the directive. |
|
Couple of obvious things to change: In the enketo service: From a performance perspective, we also read all entries I'm also not sure why we need to check against the UUID; we can only retrieve entries for the same UUID.
It looks like it is already not passed in |
Since: - it is redundant; the phone only has information for one user anyway - it is currently unused e-mission/e-mission-docs#727 (comment)
|
To fix the issue with reading all the entries again, we need to be a bit careful. The Let's make sure to store the full datastructure there. |
Since we already match the user input for the trip during populateInputsAndInferences, we don't need to re-pull all the entries and re-match during the survey launch. This is likely to be particularly bad if we collect data over a long period of time. Reading all the user inputs for a year every time we want to launch the survey is quickly going to get old. This also means that we need to store the entire survey object instead of only the label when we match entries. e-mission/e-mission-docs#727 (comment)
|
Note to self: we should ensure that we support languages going forward |
|
Now, we start exploring displaying the demographic survey during onboarding. First, let's see if we can at least launch the survey from the onboarding screen as a button, so we have a nice backup. |
|
Launching from button works. Screen.Recording.2022-05-11.at.12.19.54.PM.mov |
|
I identified my previous attempts at creating the wrapper to work with the enketo form. Comparing it to the current wrapper, potentially modified by the UNSW folks, the changes are minimal. The main differences are:
|
Concretely:
- add a new survey.html which just has the inline function
- once the account is registered, move to the next slide instead of popping up the survey
- add a new `inline` directive which has all the information from the `<ion-content>` in the popup
- have the directive call `initSurvey`
- which involved having `initSurvey` exported from launch
There are still many weirdnesses that need to be fixed, but let's first get this committed
e-mission/e-mission-docs#727 (comment)
|
Aha! I have it working inline in e-mission/e-mission-phone@c987e3f. inline_demographic_survey.movI had to do some fairly hacky stuff which I will clean up later. Notably:
Let's clean each of those up one by one. |
|
Trying to convert the
works Putting the
|
|
This is probably some subtle artifact of the scrolling and the height. We don't really need any other inlining of the survey now, so let's stick to the directive starting with the |
|
One issue that I've been encountering consistently is that after we go through the onboarding screen, when we launch the popup, it doesn't show the form. If we reload the screen (e.g. by editing a file in the devapp), it works. I bet this is something subtle related to
|
|
Aha! Figured it out! The problem is that after we have gone through the intro, there are two Before launching the modal againBoth forms are populated - the modal from the previous load and in the inline from the intro screen
After launching the modal againThe form is reloaded, but the first form.or is selected, so the intro screen is loaded, not the popup. So there is no form in the popup and we can't see anyting.
Two potential solutions:
Let's explore the first option with a time bound. |
|
This is even more obvious when you go back to the intro after launching the popup. So popup -> intro (two copies) -> popup (second form added to intro) -> intro shows two forms in the slide. One of which is stuck at the last screen of the survey and the other which moves.
|
|
The phone and server changes are now done. We can just merge the server code to master. Since we just merged from master, the chances of that happening are low, but don't want to introduce regressions at this point. We probably want to do something like: |
|
Looking at the actual set of changes in the PR, most of them are in the
So maybe we should see what happens if we try to merge everything to master. |
|
Comparing to master, we have:
Everything from May of this year is code that I want to keep |
|
Those initial changes are primarily to most of which (intro, consent, etc) I will need to change back to non-RCITI values Although those are a good template of what to modify while merging to master. |
|
Looks like there are basically two options:
cherry-pick is not considered a great option because the commit IDs will all be different and we could end up with merge conflicts But once we have merged this to master, we can abandon the wrt "cherry picking a commit from one branch to another basically involves generating a patch, then applying it, thus losing history that way as well." Most of the code is in new files, so not sure we are losing a lot of history. Let's try it on a new branch anyway and see what it looks like. |
|
This doesn't sound very promising. The very second commit fails. |
|
Merging directly into master does not result in any merge conflicts We could just merge into this subsidiary branch and revert all the UNSW-specific changes that we don't want. |
|
Let's go with that for now; a few additional commits in the history are not too terrible and at least then we don't need to deal with merge conflicts. Ok so the new plan is:
|
|
reverting individual changes results in merge conflicts, especially if they have been modified by subsequent changes. Trying to revert a range the help around this is
|
|
reverting all the way to the end to avoid partial reverts gives us one correct revert, and one conflict. |
|
After resolving that conflict, moving on to the next revert. |
|
Ah that is because of https://stackoverflow.com/a/65638479/4040267 |
|
Went through and reverted all changes to the best of my ability.
to figure out which changes need to be retained |
|
Painstakingly went through, reverted all UNSW-specific changes and re-introduced only the changes needed for this program. Disposition of changes
Current status:
Let's go ahead and merge the reverts for now |
|
Fixing remaining minor issues: If there are no user inputs saved (ever), we get this error.
|
|
Next, buttons are not covering the entire row. During testing, we had width=100%, but we reverted it because that is not true for all trip buttons.
|
|
Adding a text-align style, similar to the multi-label buttons centers it and adding a width of 100% makes it take up the entire row. Note that the width needs to be on the button, not the parent DIV to override the hardcoded width in the diary.css
|
|
Remaining issues:
Not even for the multi-labelScreen.Recording.2022-05-26.at.8.18.27.PM.mov |
First issue fixed
Second issue fixedScreen.Recording.2022-05-26.at.8.35.30.PM.movThird issue fixedlabeling_from_label_screen_working.mov |
So that it looks proper It centers the button and ensures that it takes up all the width of the item. e-mission/e-mission-docs#727 (comment) and e-mission/e-mission-docs#727 (comment)
Where if the user input list has a single entry, we can return it. If we have one user input and multiple trips, we can't just return that one input. That is the input for one trip, not the others! e-mission/e-mission-docs#727 (comment) wrt performance, we already handled the major performance issue by only having to manually match unprocessed entries after the end of the pipeline Testing done: - loaded a day with a single user input - before this change, all trips were flagged with that input - after this change, only the first was
Before this, we used to return the Promises.all and fill in the map in its then. As part of the refactoring for the enketo trip labels, aec0051 we changed it to return an array of the result and the Promise.all This meant that the infinite scroll code, which was expecting a result map, got a promise array instead, and so the mapping of user inputs in the infinite scroll list = label list failed consistently. Fixing this by re-returning `Promise.all` and the result and mapping from the `then` Note that the labels from the diary were not visible on the label screen even for the classic multilabel UI e-mission/e-mission-docs#727 (comment) Original code: ``` return Promise.all(manualPromises).then((manualResults) => { const manualConfirmResults = {}; manualResults.forEach(function(mr, index) { manualConfirmResults[ConfirmHelper.INPUTS[index]] = mr; }); return [result, manualConfirmResults]; }); ``` Broken code: ``` return [result, Promise.all(manualPromises).then((manualResults) => manualInputFactory.processManualInputs(manualResults, manualConfirmResults))]; ``` Fixed code: ``` return Promise.all(manualPromises).then((manualResults) => { manualInputFactory.processManualInputs(manualResults, manualConfirmResults); return [result, manualConfirmResults]; }); ```
By handling the different trip structure formats in the diary and the label view e-mission/e-mission-docs#727 (comment)
|
Note that the related server fix is at: |
To use the behavioral data, we need to have a demographic survey for each user. We have typically used external surveys (Qualtrics/Google Forms) before, but those have the following limitations:
A solution that would address all those issues is to store the survey information in mongodb, just like everything else. But we don't want to create a survey builder. Instead, we will use kobotoolbox to create a survey which we will display using the enketo library.
The UNSW group has already integrated with enketo core in the https://github.com/e-mission/e-mission-phone/tree/rciti branch, so let's start by exploring that approach.
If we can generalize this sufficiently, we can also have a config option to create surveys for each trip instead of using the labeling approach.
The text was updated successfully, but these errors were encountered: