Upgraded rciti #825
Open
Upgraded rciti #825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See e-mission/e-mission-docs#669. This should hopefully be a fix, but the problem occurs so inconsistently that I can't be sure. Steps taken: + Select walkthrough targets by ID + Remove incorrect use of IDs to make room for correct use of IDs + Use mildly hacky CSS to exclude DOM elements that have been translated to negative X, which often happens with the duplicated ones + Scroll back down only at the end of the walkthrough so we can back to the previous step more often + Fix walkthrough launch button padding Testing done: + Tested on an emulated iPhone 8 logged in with my real stage token - I had previously often been able to reproduce the problem under these conditions, and now I'm not seeing the problem, but it could just happen to not be appearing
Albeit in a lamer fashion. The problem is that there are multiple elements with the same ID in the list. We could consider switching to a more sophisticated walkthrough solution. However, for now, we just step out to the full list of trips instead of highlighting individual items. This seems to work pretty reliably, let's push it to staging and see if it works.
Cherry-pick the walkthrough changes to master
Testing done: - Load data for 2016-08-04 - Visit the day in the UI - verify that the `<multilabel>` element is rendered ``` <multilabel inputs="['mode', 'purpose']" class="ng-binding">inputs are ["mode","purpose"]</multilabel> ```
…eholder
Move out the HTML for the button bar into "multi-label-ui.html"
Move out the controller code to manipulate the popup and save the resulting data
Add a new input for the list of valid labels since the code currently resets the list
```
$scope.inputParams = {}
```
so we want to do that at the screen level, and not the individual directive level.
Next steps:
- Figure out how to properly refactor the remaining references to
ConfirmHelper.INPUTS, notably related to:
- `$scope.$on('$ionicView.loaded', function() {` and
- `$scope.populateInputFromTimeline(`
+ To allow us to move user input labeling into the directive as well (e-mission/e-mission-docs#674 (comment)) This was a little tricky because it turns out that when the directive is created, `tripgj` is undefined. It then gets updated through the digest process. The really weird thing is that when we log the `scope` object directly, it looks like it also gets updated with the digest. So we ended up with logs like ``` Scope isolateBindings: {inputs: {…}, tripgj: {…}, unifiedConfirmsResults: {…}, inputParams: {…}} inputParams: {MODE: {…}, PURPOSE: {…}} inputs: (2) ["MODE", "PURPOSE"] tripgj: {data: {…}, start_place: {…}, style: ƒ, onEachFeature: ƒ, pointToLayer: ƒ, …} unifiedConfirmsResults: {MODE: Array(60), PURPOSE: Array(63)} userInputDetails: (2) [{…}, {…}]__proto__: Object unifiedresults [object Object] scope trip information is undefined ``` which was driving me crazy. After adding the `$watch` callback, we can now fill the inputs correctly `` the trip binding has changed from [object Object] to new value Checking to fill user inputs for [object Object] potentialCandidates are 2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1632745829.985,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) drove_alone logged at 1632745826.26,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1602636485.509,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1602636496.041:undefined ``` + Check in the multi-label-ui for the first time. Better late than never!
- Remove the unused link function - Simplify logs by removing several of them and reducing the intensity of others (by not logging the entire object)
so it takes up the entire space in the card like it used to before these changes started
it is fairly complex, so better to pull it out instead of having it be inline with the directive definition. Now that we know how to include it, we can even move it out to a separate file if it gets really complicated.
Fortunately, all factories and services are already singletons. e-mission/e-mission-docs#674 (comment) The property is read asynchronously, though, so we have the data that we want to read stored as a promise. The first time we access the field, the async operation completes and the promise completes. In subsequent calls, the promise is fulfilled and will return immediately. This allows us to retain the desired singleton behavior even for an asynchronously read data source. Testing done: Promise executes first and only time ``` Starting promise execution with {} Promise list (2) [Promise, Promise] Read all inputParams, resolving with {} controller DiaryListCtrl called ``` It is then available later but without re-reading or delay ``` While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} ``` Introduced a timeout while reading the config ``` diff --git a/www/js/tripconfirm/trip-confirm-services.js b/www/js/tripconfirm/trip-confirm-services.js index 8089362e..291184fa 100644 --- a/www/js/tripconfirm/trip-confirm-services.js +++ b/www/js/tripconfirm/trip-confirm-services.js @@ -1,5 +1,5 @@ angular.module('emission.tripconfirm.services', ['ionic', 'emission.i18n.utils', "emission.plugin.logger"]) -.factory("ConfirmHelper", function($http, $ionicPopup, $translate, i18nUtils, Logger) { +.factory("ConfirmHelper", function($http, $ionicPopup, $translate, i18nUtils, Logger, $timeout) { var ch = {}; ch.INPUTS = ["MODE", "PURPOSE"] ch.inputDetails = { @@ -139,8 +139,10 @@ angular.module('emission.tripconfirm.services', ['ionic', 'emission.i18n.utils', ch.INPUTS.forEach(function(item, index) { inputParams[item] = omObjList[index]; })); - console.log("Read all inputParams, resolving with ", inputParams); - resolve(inputParams); + $timeout(() => { + console.log("Read all inputParams, resolving with ", inputParams); + resolve(inputParams) + }, 60000); }); ``` The promise reading started first ``` Starting promise execution with {} Promise list (2) [Promise, Promise] controller DiaryListCtrl called ``` Then, everything was filled in ``` DEBUG:section 2016-08-04T14:18:35.840464-10:00 -> 2016-08-04T14:34:12.571000-10:00 bound incident addition ``` Several minutes after that, the promise resolved ``` Read all inputParams, resolving with {MODE: {…}, PURPOSE: {…}} Checking to fill user inputs for 10:03 AM -> 10:35 AM While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} Set MODE {"text":"Shared Ride","value":"shared_ride"} for trip id "5f8646cde070a9164325d554" ``` And the labels were automagically filled in and green. yay for promises!
Remove all vestiges of the ConfirmHelper code from `list.js`
```
$ grep ConfirmHelper www/js/diary/list.js | wc -l
0
```
Also remove the list of supported inputs from the directive to avoid
duplication with the list in the `ConfirmHelper`. Later, we should clean up
`ConfirmHelper` to read that from the config instead of putting it into code.
Or if we choose to pass it into the directive, we have to figure out how to
pass it in to the factory at creation time.
There is no point in configuring it in two places.
And using the new directive in both places Concretely: - unify the HTML code as per e-mission/e-mission-docs#674 (comment) and e-mission/e-mission-docs#674 (comment) - replace the buttons in the label screen with the new directive - modify the directive slightly to skip `populateInputFromTimeline` if no input label list is passed in. Testing done: - Diary screen shows the trips with green labels - Label screen with the "All Trips" filter shows the trips with green labels
… screen Detailed investigation into the linkage is at: e-mission/e-mission-docs#674 (comment) to e-mission/e-mission-docs#674 (comment) Also changed the formatting of the diary screen a bit to clear some space for the verify check button. Note that we needed to reduce the scope of the `toDetail` call in that case, since it will result in both confirming and going to the detail screen. Testing done: Clicked on the "VC" button in both the diary and the infinite list screens. Got the click and found the corresponding multilabel in both cases. ``` element is S.fn.init [verifycheck.col-10.diarycheckmark-container] parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)] row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)] child multilabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)] multilabel scope is Scope {$id: 2455, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …} ```
- Pass in the tagName of the label button to verifycheck - Look up element with the tagName dynamically (similar to eb1fd97) - Actually invoke a method on its scope + move the verifyTrip code from `infinite_scroll_list.js` to the multi-label code so that it can be invoked from the scope. + move the verifiability initialization from infinite_scroll_list to the verify directive since it is basically initialization code + Move the verify check HTML code from the `infinite_scroll_list` HTML code to a separate template HTML so that it actually shows a check mark instead of "VC"
Move all the labeling code from the infinite scroll list to the directive
Concretely:
- remove the input detail initialization
- move out the code which initializes the inputs and replace it with the
nextTrip link
- move the manual input population code as well. this is now a separate
implementation from the diary initialization code; we will need to unify it later
- move related functions to the directive:
- `populateInput`,
- `populateManualInputs`,
- `updateVisibilityAfterDelay`,
- `updateTripProperties`,
- `inferFinalLabels`,
- `updateVerifiability`
- delete functions that were already copied from the diary, with minor
modifications as necessary:
- create `$scope.popovers`,
- `$scope.openPopover`
- `$scope.choose`
- `closePopover`
- `$scope.verifyTrip`
also copied `trip.properties.*_ts` into `trip.*_ts` so we can have the same
implementation for both data structures (e-mission/e-mission-docs#674 (comment))
In addition, in the directive, we do the following:
- add functions to find the view element, its state and scope.
- We will use this state to indicate which view we are working with, given
that we plan to support labeling assist in the diary view as well (e-mission/e-mission-docs#674 (comment))
- We will use the scope to callback to the view and filter the trips accordingly (e-mission/e-mission-docs#674 (comment))
This almost works.
The one pending issue is that of filtering to set `$scope.displayTrips`. In
`$scope.setupInfScroll`, we read entries from the server
(`$scope.readDataFromServer`), which sets `$scope.data.allTrips`. It then calls
`$scope.recomputeDisplayTrip` which uses the userInputs to decide which trips
should be in `$scope.displayTrips`.
The problem is that we set the userInputs in the directive. However, the
directives are not executed until they are displayed. So until we compute which
trips go into `displayTrips`, we will not execute the directives, which means
that we will not have the fields we need to compute the directives. We need to
resolve this circular dependency.
Inject the filtering factory dynamically Remove the static dependency Initialize the selected filters from the factory This addresses e-mission/e-mission-docs#674 (comment)
Pull out all the populateXXX and recalculateXXX functionality into a separate service so it can be invoked from multiple controllers. This is consistent with e-mission/e-mission-docs#674 (comment) We load the service dynamically to reduce the hardcoded dependency issue. We retain a couple of simple wrappers e.g. `fillUserInputsGeojson` in the controller so that we can invoke it from `$watch` more easily and we can call `$scope.$apply` to update the visualization. Remove references to `scope.manualResultMap` from the controller since the expanded values will be pre-populated. Pass the viewScope around as needed to support the callback after the update. Add a new NOP callback to the diary view to match the desired external interface. Add blank inferred label datastructures for the diary pending a bigger restructuring that will send inferred labels to as part of the geojson.
…e `$watch` Add a simple wrapper (`populateInputsDummyInferences`) to the service to iterate over all the inputs and populate them, similar to the existing `populateInputsAndInferences`. This is consistent with e-mission/e-mission-docs#674 (comment) Dynamically inject the service in the diary as well Call the new method while filling in other fields Move the code to try and unify the trip structure to the new function as well Testing done: Scrolled through the diary and list views, do not see any performance degradation
- remove the duplicate implementations of the code that populates entries - call `populateInputsAndInferences` directly - fill in the fields needed for it to work ahead of time Testing done: - Loaded diary, no errors - Loaded label, no errors
Since `getUserInputForTrip` is now called only from `populateManualInput` we can adapt it to take the trip inputs directly, and we don't have to massage the input to meet expecations The one minor ordering change to accomplish this was to compute the basic classes before matching the inputs so that we can use the `isDraft` variable directly. We don't (currently) show draft trips on the label screen, so it will be undefined there. And undefined != true, so it will only apply for diary trips.
- Remove the unused dependency on `$q` (we use built-in promises instead) - Remove the page reload on background sync which was commented out a long time ago; we may also be removing/refactoring the background sync anyway - Remove `explainDraft`, `makeCurrent` from the label screen since we don't show draft trips there - Remove `prevDay`, `nextDay` from the label screen since we don't use day-based navigation - Remove the current trip code from the label screen since we don't have a way to launch it - Remove the code around groups which seems to have been added in (1e95720), has no documentation, uses `good1`, `good2` and `good3` as the groups - Add client stats to the diary code, similar to the label view - Remove unused `getTimeSplit` from both screens - Remove the code to refresh the maps in the diary view since we have removed that button - Remove unused `isNotEmpty` - Ensure that we don't consider the position to refresh since we don't pull to refresh any more
Remove all the references from the diary helper to `$scope` variables. These were added back when we displayed common trips (c393e3d) in order to avoid duplicating the common trip information between the list view and the common trips code. We then used them directly in the UI to format data for display. However, as part of 30561b9 we changed the template to populate values from trip variables instead of `$scope` functions. So this reassignment is no longer required. Let's remove them and replace them with direct calls to the diary helper.
In a96194d, we removed the references to `DiaryHelper` methods in `$scope`. In this follow-on, we change `populateCommonInfo` to invoke calls directly from the DiaryHelper instead of the `$scope`. Testing done: - Does not generate errors while populating the list view
- survey/external: externally launched surveys - survey/multilabel: multiple labels - survey/enketo: (in the future) for eneketosurvey - survey/hexmap: (in the future) for itinerum surveys + Fixed all paths in index.html + Changed all the module names + Changed all module imports in the files that used them
- We move out the templates from `tripconfirm` to `survey/multilabel` and change all the template loading code as well. - We also create a top layer `survey.js` which imports the relevant modules from the survey implementations. This allows the code which uses the values e.g. `list.js`, `infinite_scroll_list.js` and loads the survey dynamically, to include the survey module directly and not have to change it to switch between implementations.
- Create a "Config" in the top-level survey module for the multi-label survey
- Switch the list views to use the configurations to figure the service and filters
- Switch the HTML to use the configurations to the extent possible. This extent
is the linkedtag attribute of the `verifycheck` element. Alas, we can't
appear to set the element directive tag name directly (see )
e-mission/e-mission-docs#674 (comment)
to
e-mission/e-mission-docs#674 (comment)
+ move the one-click `verifycheck` button outside of `multilabel` since it is explicitly designed for use with other kinds of surveys as well
- Remove the old one-click-button!! - Change the module name - Remove unnecessary dependencies Current hardcoded status is: ``` grep -r multilabel www/ | egrep -v "survey/multilabel|posttrip" www//js/survey/survey.js: "emission.survey.multilabel.buttons", www//js/survey/survey.js: "emission.survey.multilabel.infscrollfilters", www//js/survey/survey.js: elementTag: "multilabel" www//templates/diary/list.html: <multilabel class="col" trip="tripgj"></multilabel> www//templates/diary/infinite_scroll_list.html: <multilabel class="col" trip="trip"></multilabel> ```
- Remove logging that happens on every directive init to improve performance
- Fix the template URL for the verify button
- Fix the case on the `linkedTag`
- Remove unused scope variables from the verify button, both in the
definition and in the usage
Testing done:
Clicked the verify button; it worked
$element is S.fn.init [verifycheck.row] linkedtag is multilabel
index.html:145 parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 child linkedlabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
index.html:145 linkedlabel scope is Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: Scope, …}
index.html:145 About to verify trip 1470341031.235 -> 1470342912 with current visibilityalready-verified
$element is S.fn.init [verifycheck.col-10.diarycheckmark-container.center-vert.center-horiz] linkedtag is multilabel
index.html:145 parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 child linkedlabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
index.html:145 linkedlabel scope is Scope {$id: 2258, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
index.html:145 About to verify trip 1470882349.3511817 -> 1470883723.49 with current visibilityalready-verified
This is a bit hacky because it assumes that the only input is the trip object. There are likely ways to get around it through clever dom manipulation by using the `link` method but I am not going to overengineer here. This is basically the `ng-if` idea outlined in: e-mission/e-mission-docs#674 (comment)
Fix multiple map-related issues
This fixes the second issue in e-mission/e-mission-docs#722 (comment) In particular, we add the `viewport-fit=cover` tag. Unfortunately, since we are using ionic version 1, it doesn't automatically set all the safe areas correctly. So we had to figure out how to send them manually. Finally got it to work, applied it only to iOS e-mission/e-mission-docs#722 (comment) Both iOS and android work well now, at least in profile mode e-mission/e-mission-docs#722 (comment)
- notification check: e-mission-data-collection
e-mission/e-mission-docs#722 (comment)
- crashes e-mission-transition-notify, cordova-usercache
e-mission/e-mission-docs#722 (comment)
…reen This fixes e-mission/e-mission-docs#658 Thanks to @allenmichael099 for reporting a reproducible use case that I could fix in under an hour
Minor fit and finish issues, primarily on iOS, but one for android
I would normally not push this, but we missed one overlay hide when generating an infinite scroll event. + add additional timing information so we can see the cause of the delay on my personal phone and where we may want to move the overlay code
Add additional logging + one more overlay hide
By converting the popover into a separate view, similar to the detail view. Presumably, this does something magical with the scope, which prevents the unfortunate spillover of some kind of invalidation into the list view. Issue reproduction: e-mission/e-mission-docs#111 (comment) e-mission/e-mission-docs#111 (comment) Potential fixes (none of which worked): - Removing invalidateSize: e-mission/e-mission-docs#111 (comment) - Trying to make the map id depend on the index: e-mission/e-mission-docs#111 (comment) - Changing from `collection-repeat` to `ng-repeat`: e-mission/e-mission-docs#111 (comment) - pass in the trip object directly: e-mission/e-mission-docs#111 (comment) Final solution: - Create a map of confirmed trip objects in the timeline - Reset it every time we modify the list of trips - Create a new simplified detail screen without sections - Look up the confirmed trip, convert it to geojson and display it - Copy over the id for the retrieved confirmed trips to ensure that we have a unique key Bonus fix: Handle an undefined input for the trip -> geojson function correctly Testing done: - Loaded data for day in diary view - Clicked on detail from infinite scroll view - Went back to diary view - Everything was fine
So you don't have to see the trip and then go back to the list to verify Fortunately, this was already a directive, so the change largely involved adding the directive to the detail screen. Couple of modifications: - we allow the specification of a linkedid to make the DOM traversal simpler - we choose the survey opt in the detail screen as well - on `recomputeDisplayTrips`, we close the detail screen TODO: Might want to configure the wait to recompute since 30 secs is too large for the detail screen use case. e-mission/e-mission-docs#111
This is a pretty straightforward extension to 7d98d5d
Because it doesn't appear anywhere else
…secs Summary of changes: - create a new attribute called `recomputedelay` - plumb it through `linkedsurvey` -> `multi-label-ui` directives - set it to a default of thirty secs if it doesn't exist, and convert secs -> ms - pass it through from the controller to the service - use it in the service in place of the hardcoded value
Summary of changes: - read in the filter checks for the survey option - run all of them and go back to the list view if they are false (e.g. filter is false; would not be displayed) - change the recompute delay to 5 secs for the detail screens so the screen disappears fairly quickly Note that the check in the diary is fairly hacky since the trip doesn't have all the information required for the toLabelCheck and it always succeeds
Since setting them causes the maps in the list view to break: e-mission/e-mission-docs#111 (comment)
e-mission/e-mission-docs#111 (comment) Principled fix is being tracked in: e-mission/e-mission-docs#726 Change: - If we receive an invalidateSize when we are hidden (size == 0), then we ignore it instead of moving the view to match
Since an unspecified value returns an empty string ("") and is not undefined.
How that we have finally figured out and fixed the broken maps issue Originally commented out due to: e-mission/e-mission-docs#111 (comment) That issue was fixed in 2b72468 (e-mission/e-mission-docs#111 (comment)) So we can now restore this useful fix + reduce the delay to help with impatient people :)
… screen and vice versa Since the diary and label screens are not currently unified, values set in one screen are not reflected in the other. This is confusing if people switch between the two. Hacking around this by copying the user inputs from the diary to the list and vice versa after enter. Changes: - new function to copy input if newer - add a write_ts to modified user inputs to track which is newer - call the new function for all trips in the diary and for trips that are loaded in the label (since the label has more trips than the diary).
Fix the weird issue where the map popup breaks maps in the diary list
Bump up the version numbers to address the new map fixes
…into upgraded_rciti
- we pass in the user input label, so use it directly instead of retrieving it from the user input - break at the right location to fill in the user input label into the trips user input field Testing done: - Labeled a couple of trips
Since it won't work with the survey results anyway
|
Notes from the merge: Merge changes (conflicts/deletes)Other changes and how I handled them: Mismatch with trip_confirm_optionscaused while loading options since However, this is not a breaker since we don't use the inputs in the options lack of label viewComment out the |
shankari
added a commit
that referenced
this pull request
May 9, 2022
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
|
@asiripanich any updates/thoughts on this? I am almost done with #826. If I don't hear from you, I will probably just merge that directly to master instead of merging to rciti first. |
Hi Shankari, I no longer run an e-mission server for the rCITI branches. Happy to help you test on a test branch that you have. |
|
@asiripanich do you plan to use the rciti branch in the future? If not, I could just skip the test of this change and move on to testing the refactored code on master. I don't have an ad-hoc server running any more either. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@asiripanich pulling from master to the rciti branch to prepare for fixing e-mission/e-mission-docs#727
Please let me know if you see any issues.
Testing done below:
Screen.Recording.2022-05-06.at.7.24.20.AM.mov
You might also want to upgrade your app to this version since it includes the app status screen, labeling directly from the diary view, and some map improvements (no more gray tiles!)
For e-mission/e-mission-docs#727, I will modularize the survey components as I has suggested to @atton16 so such merges should become simpler. But if we do end up using enketo for onboarding, we will maintain it anyway.