Skip to content

Conversation

@MartinRiese
Copy link
Contributor

@MartinRiese MartinRiese commented Nov 5, 2025

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 shouldRequestLocation but only for navigate_menu requests. 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 the HereDummyFunc was used already. For the details the get_details request had shouldRequestLocation set to true but 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

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

It is not implmented on the web apps side, nor is there a plan for it
@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.36%. Comparing base (969757f) to head (696b310).

Files with missing lines Patch % Lines
.../formplayer/services/MenuSessionRunnerService.java 75.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

public EvaluationContext getEvalContextWithHereFuncHandler() {
EvaluationContext ec = sessionWrapper.getEvaluationContext();
ec.addFunctionHandler(new FormplayerHereFunctionHandler(this, currentBrowserLocation));
ec.addFunctionHandler(new ScreenUtils.HereDummyFunc(-23.56, -46.66));
Copy link
Contributor

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.

Copy link
Contributor Author

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().

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@MartinRiese MartinRiese marked this pull request as ready for review November 19, 2025 20:15
@MartinRiese
Copy link
Contributor Author

There are some web apps in production that use the here() function and are getting the dummy value. We should not break their apps.

https://docs.google.com/spreadsheets/d/1nQSnugJBreKLzxCoH8JAVGdG8MC_bL1OKaUDW6-i5YM/edit?usp=sharing

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.

3 participants