Dynamic surveys - Githubissues

sebastianbarry commented 1 year ago

Adding survey-data for each different survey into this repo (they were removed from being local to the e-mission-phone repo)

Adding surveys info into each of the study/program's config files - These are still WIP, so let me know if you think there is a better way you believe they can be structured/missing data please!!

sebastianbarry commented 1 year ago

@shankari and @JGreenlee Here is my first idea for how the surveys field will look on the deploy-config files. Let me know if this is how you'd imagined it, or if there's anything I missed or should add/change

sebastianbarry commented 1 year ago

I also do not know how to remove commit 7b59d0b, Git is giving me some issues when I try to drop it via the terminal:

I will pick back up on this tomorrow

sebastianbarry commented 1 year ago

Also, because I have changed the name of the field in the configs from survey_paths to surveys, the changes I made for the sebastianbarry/e-mission-phone branch places-survey will no longer work, this is also something I will need to revisit tomorrow to fix

shankari commented 1 year ago

wrt https://github.com/e-mission/nrel-openpath-deploy-configs/pull/7#issuecomment-1362216180, why do you want to remove that commit? I have had success removing the last commit in a sequence, but given that the commit history is linear, I am not sure it is easy to remove the first commit in a commit chain.

My suggestion would be to make a new branch off the current master and then cherry-pick all the commits that you want to keep. You can specify the range of cherry-picked commits, and since they are all fairly new, they should apply cleanly.

shankari commented 1 year ago

I know that I said that we should potentially move the enketoSurveyConfig.json to the dynamic config and remove it from the phone directory, but after seeing this, I think that we may want to keep the TripConfirmSurvey and UserProfileSurvey unchanged since (1) we don't need them to be dynamic just yet and (2) changing the TripConfirmSurvey in particular will be a problem for the UNSW group.

So for now, we should load the new trip-additions and place-additions surveys from the dynamic config and keep the TripConfirmSurvey and UserProfileSurvey from the existing enketoSurveyConfig.json

sebastianbarry commented 1 year ago

To-do:

[ ] Move the survey-resources stuff from e-mission-phone to this surveys direcotry
[ ] Move it to the dynamic config, but make the dynamic config have the ability to load from the local file system (dynamicConfig.js will load it from the dynamic config from online, instead of loading it locally), while also being able to be loaded externally from github (continue working assuming this is the standard)

shankari commented 1 year ago

I changed my mind again - if we put the new configs into the dynamic config, then UNSW will have to deal with dynamic configs anyway. So I think that we should change the dynamic_config.js to be able to read from local or remote dynamic configs and maybe even change the remote dynamic config location to support other apps.

So we go back to putting everything into the dynamic config.

        const downloadURL = "https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/"+label+".nrel-op.json"

shankari commented 1 year ago

two options are:

TimeUseSurvey: {....}
place-level-additions-survey: TimeUseSurvey 
trip-level-additions-survey: TimeUseSurvey

OR

place-level-additions-survey: {
  formURL: 
  version:
  ...},
trip-level-additions-survey: {
  formURL: 
  version:
  ...}

We will go with 1, since then we can define the survey once and use it with multiple button locations

JGreenlee commented 1 year ago

@sebastianbarry @shankari

Here's how I suggest we can go about generalizing and renaming the "time use" functionality.

enketo-time-use-button becomes enketo-add-note-button. It will take an argument 'survey'.

trip_list_item.html: <enketo-add-note-button survey="trip-notes"> place_list_item.html: <enketo-add-note-button survey="place-notes">

config structured like this

...
"surveys": {
    "TimeUseSurvey": {
        "formPath": ...
        ...
    }
    "place-notes": TimeUseSurvey 
    "trip-notes": TimeUseSurvey
}
...

In the controller, we read $scope.survey, find it in the config, and lookup the appropriate URL

const surveyKey = $scope.ui_config.surveys[$scope.survey];
if (!surveyKey) {
  console.log(`No survey found for target ${$scope.survey}. Skipping...`);
  return;
}
const survey = $scope.ui_config.surveys?.[surveyKey];

// then load by survey.formPath

shankari commented 1 year ago

@JGreenlee that sounds good in general. One final design question - where will the ng-if to show/hide it be? Note that if place-notes is not present, for example, we do not want to display the place directive at all. In that case, the label screen should be a list of trips.

Your original plan was to put it outside the directive (https://github.com/sebastianbarry/e-mission-phone/pull/1#discussion_r1054013451). How would that work with this plan?

JGreenlee commented 1 year ago

@shankari I think either way, the config needs to be accessed in at least two places.

We have this nested structure:

infinite_scroll_list

place_list_item

enketo_add_note_button

The enketo_add_note_button needs to read dynamic config to get the appropriate formPath and labels.

And the ng-if to hide/show has to be either in infinite_scroll_list or place_list_item Right now I'd still lean towards putting that logic in infinite_scroll_list - especially because that's where we also need to dynamically set item-height to get the collection-repeat working. (If place listitems are being shown, then the heights will be increased)

If there's a way we can avoid this boilerplate and just read the config once, that would be better - but I don't know of one (Can we keep a global instance of ui_config? Or would that bad practice in angularjs?)

shankari commented 1 year ago

There is a global instance of ui_config - in www/js/config/dynamic_config.js https://github.com/e-mission/e-mission-phone/blob/master_for_platform/www/js/config/dynamic_config.js#L81

The problem is that the global instance is read from the SQLite database, so it is not guaranteed to be available at the time that the directive is initialized, particularly if it is the first tab that the user interacts with. Only after the native plugins are available can the call to the database be initialized, and only after the asynchronous call to the database returns is the config available.

When I originally thought about this, I assumed that the list would listen to the config and that it would pass the survey info through to the enketo button directives through a parameter. That seems like a reasonable balance between a hard-coded config and reading the config everywhere.

aka in infinite_scroll_list.js

    $ionicPlatform.ready().then(function() {
      DynamicConfig.configChanged().then((newConfig) => {
        $scope.$apply(() => $scope.ui_config = newConfig);
      }
   }

in infinite_scroll_list.html

                <infinite-scroll-trip-item trip="trip" config="ui_config" map-limiter="mapLimiter"></infinite-scroll-trip-item>

JGreenlee commented 1 year ago

@shankari Passing it down through parameters looks like a better solution, thanks I had been retrieving it via that promise each time, so I'll change it tomorrow

shankari commented 1 year ago

Remove node_modules
now that we don't have the enketoSurveyConfig.json any more, every single production deployment should have the UserProfileSurvey

sebastianbarry commented 1 year ago

This PR can be closed as I created a new PR which removes the Denver-CASR commit and is overall much cleaner and solves the problem more concisely: https://github.com/e-mission/nrel-openpath-deploy-configs/pull/8

e-mission / nrel-openpath-deploy-configs

Dynamic surveys #7