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

Refactor enketo into a directive for use in both the trip and onboarding screens #826

Closed
wants to merge 35 commits into from

Conversation

shankari
Copy link
Contributor

@shankari shankari commented May 7, 2022

No description provided.

shankari added 3 commits May 6, 2022 13:56
These are `www/js/survey/enketo` and `www/templates/survey/enketo`
Change:
- Create a new directive called `enketo-trip-buttonA`
- Create a controller and service for it based on the multi-label code
- Remove most of the multi-label code from the new controller
- Plumb it through the survey selection code
    - Add a new entry for ENKETO
    - Handle the new tag in the wrapper
    - Change the ng-if to match

Testing done:
- Clicking on the button launches the survey
- Survey completes correctly
- On survey completion, the button is updated correctly

Need to be fixed:
- Position of button
- Initial population of the button label with already filled in results
@shankari
Copy link
Contributor Author

shankari commented May 7, 2022

State after b6ad175

The details label and button now cover the entire width
@shankari
Copy link
Contributor Author

shankari commented May 7, 2022

State after 8aa8900

This was just reading from the hardcoded `userInput` `'SURVEY'` value instead
of iterating over all of them.
@shankari
Copy link
Contributor Author

shankari commented May 7, 2022

Current state after 0636a90

@shankari
Copy link
Contributor Author

shankari commented May 7, 2022

As of now, 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.

shankari added 16 commits May 9, 2022 09:48
Now that we have a separate directive for it (b6ad175)
…w enketo directive

We could theoretically replace the user input array by a single value but we
retain it as an array with a single, hardcoded value. We do remove the
iteration of the array and access it directly using the single, hard-coded
value. This allows us to support incremental changes, and defer the more
complex refactoring until later.

Similarly, we add early returns to all the verification code to convert it to a
NOP and ensure that it is not used. We should remove it before finalizing this
PR since we don't want to have copy-pasted and unused code lurking around in
the codebase. We can restore it from the multi-label-ui codebase when we do
start supporting guesses for the surveys as well.

```
$ grep -r ConfirmHelper www/js/survey/enketo/
Binary file www/js/survey/enketo//.enketo-trip-button.js.swp matches
```
I added this as part of the
#825 PR to try and duplicate
this special case handling of the enketo code
https://github.com/e-mission/e-mission-phone/blob/59690cb503c682cf09b3681db70aeafa8d4631c0/www/js/diary/services.js#L1127

However, I actually ended up handling it at
https://github.com/e-mission/e-mission-phone/pull/825/files#diff-4d928ea1f0def79e554ae472efcde3c3cfb464593833e77b9bc4dae5017bd244R1140 (in `readTripsAndUnprocessedInputs` instead)

So this NOP `if` statement is no longer required
… deal with the manual inputs

This ensures that we can more easily move the implementation into the
individual directives in the next commit.

Testing done:
- Loaded data from a previous day
- Labels were assigned correctly
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)
And use it to map the manual input results into a map indexed by key
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)
Since:
- it is redundant; the phone only has information for one user anyway
- it is currently unused
e-mission/e-mission-docs#727 (comment)
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.
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)
This should allow us to load and store the demographic survey as well
- Restore a sample user profile.json
- Create a new demographic button directive
    - the control launches the survey in a popup
    - the service reads all surveys and picks the most recent one
- change the control screen to use the new button instead of a one-off launch command
- remove the callback for the one-off launch from the control code
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)
@shankari
Copy link
Contributor Author

Working demographic survey, including save and reload

Screen.Recording.2022-05-11.at.11.09.33.AM.mov

.

shankari added 3 commits May 11, 2022 22:22
If the user "logs out" and goes back to the intro screen, it is re-created.
This ensures that we don't have two form fields sitting around right after we
have finished the onboarding, and that opening the popup will not add a new
form to the wrong form field

This fixes e-mission/e-mission-docs#727 (comment)
e-mission/e-mission-docs#727 (comment)
e-mission/e-mission-docs#727 (comment)
e-mission/e-mission-docs#727 (comment)

We tried to remove only the form directive but in that case, the form is not
re-created if we go back to the intro screen.
e-mission/e-mission-docs#727 (comment)

Deleting the entire view does work
e-mission/e-mission-docs#727 (comment)
To avoid duplication of content. We can theoretically generalize the
demographic button as well, but running out of time for additional refactoring
now.
@shankari
Copy link
Contributor Author

Current approach of pulling out the <ion-content as the common functionality (ff7863d) is a bit complicated since it pushes the additional controls (skip-button, etc) to the intro controller rather than keeping it isolated in the component. Let's refactor so that only the form is pulled out and ng-include it in both the modal and the inline directives.

@shankari
Copy link
Contributor Author

Also, for the record, I tried to add <ion-header and <ion-footer elements outside of the <ion-content in the slide - e.g.

Header and footer
<ion-header>
<div class="consent-title">
<center><h2>Demographic Survey</h2></center>
</div>
</ion-header>

<!--
<div ng-if="existingDemographicSurvey && !editSurvey" class="col" style="margin-top: 20px;" translate-namespace="survey">
  <div class="row" style="margin-right: 5px">{{'.prev-survey-found'}}</div>
  <div class="row">
      <div class="col" style="margin-right: 5px">
          <button class="button button-block button-assertive button-outline" ng-click="finish()" id="consent-button"><span translate>{{'.use-prior-response'}}</span></button>
      </div>
      <div class="col" style="margin-left: 5px">
          <button class="button button-block button-positive button-outline" ng-disabled="!overallStatus" ng-click="setEditSurvey(true)" id="consent-button"><span translate>{{'.edit-response'}}</span></button>
      </div>
  </div>
</div>
-->

<enketo-demographics-inline ng-done="finish" existing-survey="existingDemographicSurvey"></enketo-demographics-inline>

<ion-footer>
<div class="col" style="margin-top: 20px;" translate-namespace="survey">
  <div class="row" style="margin-right: 5px">{{'.prev-survey-found'}}</div>
  <div class="row">
      <div class="col" style="margin-right: 5px">
          <button class="button button-block button-assertive button-outline" ng-click="finish()" id="consent-button"><span translate>{{'.use-prior-response'}}</span></button>
      </div>
  </div>
</div>
</ion-footer>

<div class="consent-space"></div>

results in

Footer Header
Screen Shot 2022-05-13 at 6 42 04 AM Screen Shot 2022-05-13 at 6 42 45 AM

Note that the footer overlaps the main screen and the header is under the main screen.

The UI is still clunky but the functionality works:
- if no prior response, show the survey
- if prior response:
    - by default show a message asking the user whether they want to edit or skip
    - if editing, load the survey and let them edit
@shankari
Copy link
Contributor Author

shankari commented May 14, 2022

The UI is still clunky but the functionality works:

@shankari shankari mentioned this pull request May 17, 2022
shankari added 2 commits May 22, 2022 23:24
So people can determine whether they want to edit it or not.
Unfortunately, it doesn't work very well since it displays the code versions of
the name and value.

Concretely:
- this means that the names are not very meaningful (e.g. `hh_size` instead of `household size`)
- this also means that they will not support i18nA

Checking this in anyway for now, but will remove it in the next commit
e-mission/e-mission-docs#727 (comment)
In the case that the prior survey response is found, make the text bigger and more centered
@shankari
Copy link
Contributor Author

Screen looks pretty decent now, except that the buttons don't have borders.

Screen Shot 2022-05-23 at 8 20 44 AM

Let's fix that and move on.

@shankari
Copy link
Contributor Author

I am not sure why the button doesn't have a border.
The button classes and id are:

<button class="button button-block button-assertive button-outline" ng-click="ngDone()" id="consent-button"><span translate="" class="ng-binding">Use prior response</span></button>

which are the same as the

<button class="button button-block button-assertive button-outline" ng-click="disagree()" id="consent-button"><span translate="" class="ng-binding">I refuse</span></button>

But there is no border.

running out of time to figure out why (maybe I need a directive-specific CSS), so just dumping this into the element style for now

shankari added 8 commits May 23, 2022 08:41
At this point, we are basically done with the phone functionality
This makes it easier to search through the documents in MongoDB and to access
the fields in javascript.

This is consistent with:
e-mission/e-mission-docs#727 (comment)
and is the last client-side change for this feature
In ff7863d and
df21f08 we pulled out the enketo form code
into a separate form-base.html which we `ng-include`d in both the popup and
inline versions.

This actually commits that form-base.html which we forgot to commit earlier.
Data model: e-mission/e-mission-docs#727 (comment)

This makes the format more consistent with the other entries stored on the server.
- data.ts as a numeric timestamp in seconds
- data.fmt_time as a formatted date

- data.start_ts/end_ts for trip-oriented objects
- data.ts for one time surveys that are not related to a particular trip
We can fill them in with real implementations if/when we turn on inferences for
the survey-based labels.
This ensures that it is displayed correctly
@shankari
Copy link
Contributor Author

Closing this since all these changes are subsumed by #832

@shankari shankari closed this May 27, 2022
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.

1 participant