fix(dashboard): use server-side lastModifiedTime for co-edit check #11614
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11614 +/- ##
=======================================
Coverage 62.26% 62.26%
=======================================
Files 873 873
Lines 42256 42256
Branches 3961 3961
=======================================
Hits 26310 26310
Misses 15766 15766
Partials 180 180
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Mmmh, it feels like using/comparing the client time and server time is a brittle approach as time zones and/or a wrong client clock would create problems. Could we just rely on the server time stamp and use it as the equivalent of a version number? Meaning even when we save the server could return a payload with a new server side timestamp that we can stick into the redux store and send on the next save event a an equivalent to a version id. |
Yes, i am also looking for a solution like that. Currently what is best way to get its changed_on after an update? (also need to apply same change to |
graceguo-supercat
force-pushed
the
gg-FixSaveDashError
branch
from
158fa13 to
89dec86
Compare
|
I pushed another solution: server-side returns last_modified_time (which is changed_on after latest update) in |
graceguo-supercat
changed the title
ktmud
changed the title
| session.close() | ||
| return json_success(json.dumps({"status": "SUCCESS"})) | ||
| return json_success( | ||
| json.dumps({"status": "SUCCESS", "last_modified_time": last_modified_time,}) |
There was a problem hiding this comment.
Can we rename last_modified_time everywhere to changed_on if it's not too big of a change?
Having some consistency is nice.
There was a problem hiding this comment.
changed_on is a column name in Dashboards table, its type is DATETIME. I prefer to use utc timestamp (use it like version id) so that ignore local time etc (maybe different setup in each company).
|
|
||
| # get updated changed_on | ||
| dash = session.query(Dashboard).get(dashboard_id) | ||
| last_modified_time = dash.changed_on.replace(microsecond=0).timestamp() |
There was a problem hiding this comment.
Nit: .replace(microsecond=0) is probably not needed.
There was a problem hiding this comment.
i want to use timestamp in the second level, to avoid comparing 2 float number.
|
Hey side note (totally supportive of merging this!) but wanted to try and understand better how we prevent users from colliding with themselves. I think we have many ways that we update the dashboard definition (many modals)
Do we check the version on all/some of the endpoints? |
Hi @mistercrunch All the changes from dashboard header dropdown list, go through single API Currently |
…pache#11614) * fix: [dashboard][co-edit] Round client-side lastModifiedTime * another try: server-side returns last_updated_time in save_dash response (cherry picked from commit b9284d3)
…pache#11614) * fix: [dashboard][co-edit] Round client-side lastModifiedTime * another try: server-side returns last_updated_time in save_dash response
mistercrunch
added
🏷️ bot
SUMMARY
In #11220 and #11305, we introduced a feature that prevent mid-air collision when 2 users editing same dashboard. There are some users in airbnb reported that when only 1 user editing dashboard a couple of times they will see the warning message, even there is no co-editing happens, see issue #11477
When i debug this issue, i found when error happens, it shows client-side last_updated_time and dashboard's changed_on has a very subtle mis-match, for example:
it seems dashboards table's changed_on is rounded-up to second level, while client-side last_update_time is not.
So i will add a round function in client-side and try to make this number match in the second level.
TEST PLAN
CI and manual test
ADDITIONAL INFORMATION
cc @ktmud @eschutho @mistercrunch @zuzana-vej