Table refresh with ntie #87
Merged
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
podliashanyk
added
enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
=======================================
Coverage ? 48.34%
=======================================
Files ? 15
Lines ? 848
Branches ? 0
=======================================
Hits ? 410
Misses ? 438
Partials ? 0 ☔ View full report in Codecov by Sentry. |
lunkwill42
requested changes
May 22, 2024
src/howitz/endpoints.py
Outdated
| @@ -55,6 +55,9 @@ def auth_handler(username, password): | |||
| current_app.logger.info('Authenticated in Zino %s', current_app.event_manager.is_authenticated) | |||
|
|
|||
| if current_app.event_manager.is_authenticated: # is zino authenticated | |||
| current_app.updater = UpdateHandler(current_app.event_manager) | |||
There was a problem hiding this comment.
I haven't followed zinolib, but was this functionality recently added to zinolib? In that case, you might want to change the version requirement for the zinolib dependency?
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
fbd4369
to
30300a9
Compare
UpdateHandler returns only ids of events that have been changed, without differentiating between different types of changes (event removed, event added or event updated). For Howitz being able to differentiate which change occurred on event, info about currently known evenmts must be saved
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
30300a9
to
f32dbf3
Compare
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
2 times, most recently
from
eb6837e
to
e9e580b
Compare
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
e9e580b
to
9883d93
Compare
podliashanyk
added
blocked
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
f21f250
to
ee909a3
Compare
Docd on sync startegies: https://htmx.org/attributes/hx-sync/ From the HTMX documentation on hx-sync I have gotten an erroneous notion that HTMX's "drop" strategy is by default set in the whole project. But apparently "drop" is the default only if you use "hx-sync" attribute and don't provide any strategy, ie 'hx-sync' attribute must be used on the tag in order for HTMX to start synchronizing requests on (and within) this tag. Synchronization is not a global config option in HTMX!
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
45e23b5
to
e157850
Compare
This will make the app perform one HTMX request at a time, yet queue all requests that occur during this request. Synchronizing is necessary when having NTIE refresh requested with intervals less than 30 seconds and meanwhile having other operations on events available to perform for users. Without any synchronizing in the app, if several HTMX requests update the page unexpected race conditions occur. It most often leads to none of the requests being completely finished, instead a heap of error alerts show up on the screen.
Co-authored-by: Morten Brekkevold <[email protected]>
podliashanyk
force-pushed
the
table-refresh-with-ntie
branch
from
e157850
to
dd22052
Compare
hmpf
reviewed
May 30, 2024
This was referenced May 30, 2024
hmpf
approved these changes
Jun 6, 2024
lunkwill42
requested changes
Jun 6, 2024
There was a problem hiding this comment.
My nitpicks have been mostly fixed, but something seems off with the project requirements? @hmpf?
requirements-frozen.txt
Outdated
| @@ -48,5 +48,5 @@ werkzeug==2.3.8 | |||
| # flask | |||
| # flask-login | |||
| # howitz (pyproject.toml) | |||
| zinolib==1.0.1 | |||
| zinolib==1.0.4 | |||
There was a problem hiding this comment.
Something is weird with this. pyproject.toml says zinolib>=1.1.0, so why is zinolib 1.0.4 in the frozen requirements list?
There was a problem hiding this comment.
Because we forgot to update it probably.
There was a problem hiding this comment.
With zinolib==1.0.4 I can't even get Howitz to run, with 1.1.0 this NTIE PR seems to work just fine for me :)
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.
Closes #4
Depends on Uninett/zinolib#53, Uninett/zinolib#58
Implemented changes:
get_events()endpoint with a much more efficient NTIE withUpdateHandler. See Add server poll zinolib#53.hx-sync. This is necessary when dealing with frequent requests toUpdateHandlerand meanwhile having users potentially updating some events themselves, or selecting/expanding table rows. Without the synchronization, swaps from the "competing" requests were not complete/pending indefinitely, and heap of error alerts showed up in the view.Not implemented/further work:
With the initial table refresh all table rows during refresh showed spinner indicator at the end of the row like so:
The thought for NTIE was to only show spinner on the rows that are about to be updated with NTIE (since NTIE updates are more targeted than a refresh of the whole table). Yet I decided to drop this idea and preserve current spinner implementation for following reasons:
Attention needed:
5?