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

Remove support for running validations during the client-side muting transition #9652

Open
jkuester opened this issue Nov 15, 2024 · 4 comments
Labels
Type: Improvement Make something better

Comments

@jkuester
Copy link
Contributor

What feature do you want to improve?

Spinning this out of #9544 to give it proper attention and consideration (and particularly to get @dianabarsan's thoughts). Also, this is a per-requisite for #9544 and can be shipped ahead of that issue.

TLDR, as a part of #9544 we want to remove the key:value entries from the freetext search indexes to dramatically reduce the storage space used by these indexes on device. The goal of this issue is to remove validations support from webapp because the exists validation function relies on these key:value entries.

When the muting transition executes on Sentinel, it can run a set of configured validations on the muting/un-muting report before actually executing the action. The primary purpose here is to support muting via sms workflows (where you cannot really pre-validate the form data like you can when filling out an Enekto form in webapp).

Describe the improvement you'd like

When client-side muting was introduced into webapp, support was also included to continue running these validations when muting a client-side contact. The arguments for removing this functionality now are:

  • For the most part the validations logic should instead be included in the Enekto form to prevent the user from ever submitting an invalid muting/un-muting report.
  • The parts of the validations logic that are most difficult to re-create in an Enekto form (e.g. exists) are limited in scope when running offline (since they only have access to the data on the device) and so could produce different results than if the validation was run on the server. (e.g. the exists function does not find anything when running on the device, but could find something when running in Sentinel).

Because of this, and the perceived strong likelihood that no one is relying on validations for their webapp-based muting workflows the benefits of reducing disk usage on the device seem to outweigh the possible downsides of losing validations on client-side muting transitions.

Describe alternatives you've considered

Perhaps it would be possible to trigger the validations to run in Sentinel _for client-side muting/unmuting operations, once the muting reports are replicated to the server. Then maybe the server could roll-back the operation if the report fails the validations.

@jkuester jkuester added the Type: Improvement Make something better label Nov 15, 2024
@mrjones-plip
Copy link
Contributor

two questions:

  1. I assume we don't think this is a breaking change per "perceived strong likelihood that no one is relying on validations for their webapp-based muting workflows" ? Conversely, I assume we won't ship this if we find that indeed folks are relying on these validations? (or we'll wait 'til 5.0)
  2. Have we quantified the disk savings on handsets? given the scope of this change I wanted to be sure we knew the trade offs of saved space vs invasiveness of the change.

@jkuester
Copy link
Contributor Author

jkuester commented Nov 15, 2024

we don't think this is a breaking change

Strictly speaking, this is definitely a breaking change in that it is removing functionality that previously existed. Pragmatically speaking, though, the question is if we really need to wait for a major release to remove functionality that is half-broken and probably unused.

Admittedly "probably unused" in that statement is doing a lot of heavy lifting. We don't have any telemetry in webapp around this functionality so we have no actual usage data. However, it should be possible to spot-check any configs we have access to and look at the muting config in the app-settings. Basically whenever muting.validations is populated we want all the entries in muting.mute_forms or muting.unmute_forms to have matching entries in forms. I think that would mean they are all SMS forms (and so there would not no muting/unmuting supported in webapp).

we won't ship this if we find that indeed folks are relying on these validations? (or we'll wait 'til 5.0)

If we find any cases where we have muting.validations configured for for app forms in webapp, then yeah, I think we should definitely not ship this in a minor release. We should take the opporutunity to better understand the workflows that partner is using and then we can decide if it still makes sense to remove this functionality in the next major release.

Have we quantified the disk savings on handsets?

AFICT this will be difficult to quantify with any super reliable precision since IndexedDB seems to like to do its own thing when it comes to garbage collection. However, I did a rough test with my user that has +6000 docs. Started by totally clearing all site data in my browser and doing a hard-reload of the app. Then logged in, synced all the docs, and clicked around enough to trigger the freetext indexes to be built. Then I measured the IndexedDB size and redid the whole thing again but without indexing the key:value pairs. The starting IndexedDB size with our current views was 183 MB. Without the key:value pairs the The IndexDB size went down to 140 MB (so -43 MB). So removing the key:value indexes resulted in a ~23% reduction in the total size of the CHT data on the device. Once again, this was a very rough test, but it does meet expectations based on the similar results we were seeing in Couch...

@mrjones-plip
Copy link
Contributor

Thanks Josh! I don't think this meaningfully changes anything, but great to have the full info during the discussion

@dianabarsan
Copy link
Member

dianabarsan commented Nov 25, 2024

I believe this to be a breaking change, no matter how we hard we try to squint.

and the perceived strong likelihood that no one is relying on validations for their webapp-based muting workflows

Given that we are branching out of medic-owned configurations and strongly encouraging partners and users to do their own thing, without any obligation to report back to us, we will have increasingly LESS visibility into what people are doing. So "perceptions" will not be enough to cut out features without qualifying them as breaking changes. We should assume that features are being used.

With that, I think validations for muting forms are nice, both server and client side. It sucks how we have sync and async validations that depend on data and we're forced to have them all together.

I think saving 43MB is not super significant for the client. It's the equivalent of ~30 seconds of video capture or one Android app.
Do we have proof that not having those 43MB increases app performance in a significant way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement Make something better
Projects
None yet
Development

No branches or pull requests

3 participants