-
Notifications
You must be signed in to change notification settings - Fork 10
Remove code that attempts to fetch location data #1760
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
Conversation
It is not implmented on the web apps side, nor is there a plan for it
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1760 +/- ##
============================================
- Coverage 70.36% 70.36% -0.01%
+ Complexity 2032 2017 -15
============================================
Files 257 255 -2
Lines 7995 7942 -53
Branches 755 754 -1
============================================
- Hits 5626 5588 -38
+ Misses 2087 2073 -14
+ Partials 282 281 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| public EvaluationContext getEvalContextWithHereFuncHandler() { | ||
| EvaluationContext ec = sessionWrapper.getEvaluationContext(); | ||
| ec.addFunctionHandler(new FormplayerHereFunctionHandler(this, currentBrowserLocation)); | ||
| ec.addFunctionHandler(new ScreenUtils.HereDummyFunc(-23.56, -46.66)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method be still called ? If not think we should we hard crash here instead with a IllegalStateException so that we know this code is indeed not being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are fine with getting an error when somebody does end up using the here() function, then yes. This could just be removed and all getEvalContextWithHereFuncHandler could be replaced with getSessionWrapper().getEvaluationContext().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think if we are not going to support here, it should result into a very explicit error on Web Apps UI when someone does use it instead of giving an impression that everything is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with Woody and he thinks it would be better to throw the exception.
|
There are some web apps in production that use the https://docs.google.com/spreadsheets/d/1nQSnugJBreKLzxCoH8JAVGdG8MC_bL1OKaUDW6-i5YM/edit?usp=sharing |
Product Description
It is not implemented on the web apps side, nor is there a plan for it.
There is some code in web apps that looks for
shouldRequestLocationbut only fornavigate_menurequests. The added logging showed that this has not been called in over a month.For testing I added the
here()function the a case list and case details. For the list theHereDummyFuncwas used already. For the details theget_detailsrequest hadshouldRequestLocationset totruebut was ignored by the web apps code.The documentation states that the
here()function is only implemented for Android apps. So I think it is fairly save to remove the related code from formplayer as well.Technical Summary
https://dimagi.atlassian.net/browse/USH-6280
Safety Assurance
Safety story
Tested locally
Automated test coverage
No
QA Plan
Special deploy instructions
Rollback instructions
Review