[Data Cleaning] Updates to session status view #36469
Conversation
- extract into a partial and dry things up
- i decrease this even further in a future commit to 0.5s, but I couldn’t squash the two due to other changes that happened in between
- add whitespace for readability improvement
- way too much overhead
- let the modal overlay with the status while the pending edits are visible in the background. this allows us to avoid the need of having to refresh the table constantly during the status progress. it's much easier to 'freeze' the view and let the user see the changes in the table after resuming the session
- we are no longer refreshing the table on each poll, so it's fine to increase - gives a better feeling of responsiveness and fluidness to the user
- when a new session is created/resumed
- the session status modal is triggered when the partial first loads. add a comment about this in the modal template so that it's clear how it is being triggered. - unfortunately we couldn't use HX-Trigger headers on the main view as the first loading of the whole page is not an HTMX request. as a result, it felt the cleanest to leave the triggering to the status view itself.
| {% blocktrans count count=num_records_changed %} | ||
| Finished processing changes, updated 1 case. | ||
| {% plural %} | ||
| Finished processing changes, updated {{ num_records_changed }} cases. |
There was a problem hiding this comment.
Fancy! I think I understand this, apart from count defined twice? But I'm assuming count is some special var that will factor into whether we use the singular or {% plural %} option?
There was a problem hiding this comment.
count is a special var for blocktrans, yes
There was a problem hiding this comment.
And why does it need to be both specified and then assigned as opposed to just assigned?
| @@ -14,7 +14,7 @@ | |||
| BULK_OPERATION_CHUNK_SIZE = 1000 | |||
| MAX_RECORDED_LIMIT = 100000 | |||
| MAX_SESSION_CHANGES = 200 | |||
| APPLY_CHANGES_WAIT_TIME = 15 # seconds | |||
| APPLY_CHANGES_WAIT_TIME = 3 # seconds | |||
There was a problem hiding this comment.
This is the artificial buffer right? Is 3 seconds enough 😬?
There was a problem hiding this comment.
is it? I had to argue with @kcowger we need more, but he felt 15 was too much. but maybe someone from platform will offer better advice?
There was a problem hiding this comment.
If we ultimately tie this to pillow lag then maybe it isn't too big of a deal? But I look at a graph like this which makes 3 seconds feel tough. Granted that graph is the max lag time so it paints the worst picture.
There was a problem hiding this comment.
Ultimately, we should tie to pillow lag. But the experience of making a single inline edit and waiting 15 seconds is terrible.
| @@ -10,7 +10,7 @@ | |||
| hx-post="{{ request.path_info }}{% querystring %}" | |||
| hq-hx-action="apply_all_changes" | |||
| hx-target="{% if table.container_id %}#{{ table.container_id }}{% else %}.table-container{% endif %}" | |||
| hx-swap="outerHTML" | |||
| hx-swap="none" | |||
There was a problem hiding this comment.
Just curious how does this compare to not specifying hx-swap at all?
There was a problem hiding this comment.
if you don't specify hx-swap it defaults to innerHTML which is...definitely not what we want 😅
| The buffer allows the change feed to catch up to the form submissions, | ||
| so that the user doesn't refresh and see that their data hasn't updated due to a slow change feed. | ||
|
|
||
| TODO: update this buffer (APPLY_CHANGES_WAIT_TIME) dynamically based on change feed status. |
There was a problem hiding this comment.
@AmitPhulera did some work on a PillowLagGaugeLimiter fyi. If nothing else would serve as a useful reference for pulling lag time.
corehq/apps/data_cleaning/templates/data_cleaning/status/modal.html
Outdated
Show resolved
Hide resolved
….html Co-authored-by: Graham Herceg <gherceg@dimagi.com>
Technical Summary
Note: I plan to address issues with the text content of the status modal in an upcoming PR. so that bit isn't a part of this PR
Feature Flag
DATA_CLEANING_CASESSafety Assurance
Safety story
changes made to a feature flagged workflow that is undergoing user testing on staging
Automated test coverage
most important bits are tested, but more tests are coming
QA Plan
not blocking PR, as this still is feature flagged
Rollback instructions
Labels & Review