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

views.get_stats returns data in an old format #62

Closed
iafan opened this issue Oct 28, 2016 · 5 comments · Fixed by #74
Closed

views.get_stats returns data in an old format #62

iafan opened this issue Oct 28, 2016 · 5 comments · Fixed by #74
Assignees
Labels

Comments

@iafan
Copy link
Contributor

iafan commented Oct 28, 2016

When stats are requetsed via /xhr/stats/, they are returned in an old format (dictionary) whereas we now expect an array.

We need to change TreeItem.get_stats and CachedTreeItem.get_stats to match the new code.

Additionally, I'd suggest to look why we have code duplication in TreeItem.get_stats and CachedTreeItem.get_stats

How to reproduce:

  1. Shutdown RQ workers to prevent stats from updating
  2. Change something in a unit (e.g. toggle the 'Needs work' flag)
  3. Go back to the browser view

Since the data will be stale, the page will start requesting stats data via XHR, and this data will come in a wrong format, causing errors in the console (because React components get data in an unexpected format).

@iafan iafan added the bug label Oct 28, 2016
@iafan iafan assigned julen and unassigned julen Oct 28, 2016
@iafan
Copy link
Contributor Author

iafan commented Oct 28, 2016

@julen please have a look into this.

@julen
Copy link
Contributor

julen commented Nov 1, 2016

Quick update: I've looked through this today, and making the returned stats a list is straightforward.

But there is a caveat. The client needs to merge the new stats data with existing UI data (there are some properties which are irrelevant to stats and are not part of the /xhr/stats/ payload), and this exhibits a mismatch in the ordering of the children elements that have been provided. The effect is that stats are updated, but the displayed data is incorrect because merging happens for items which are not in the same order. I tried to sort all children elements as returned by all users of TreeItem, and while it works, it quickly becomes unwieldy and I would prefer not to resource to this.

Another option I'm thinking about would be to make /xhr/stats/ return the entire UI properties as returned to templates. This would include extra properties which are irrelevant when stats are dirty (e.g. pootlePath, itemType...), so the response payload is unnecessarily bigger. It also requires some extra plumbing to be able to return exactly the same data as in browsing views.

@iafan
Copy link
Contributor Author

iafan commented Nov 1, 2016

I'd just return exactly the same format of data as we do in browsing views. This should ideally simplify the code and help in the future to get rid of browser page reloads. That'd be step one.

Step two is an ability to get short patch updates (in the future, via websockets), so we will need a mechanism to push partial data for specific keys.

@julen
Copy link
Contributor

julen commented Nov 1, 2016

Okay, I'll try to match the format we use in browsing views. We'll need to have some plumbing code to be able to get all the context-relevant data. At the same time, I'm thinking that this won't be stats anymore, but rather something like browsing data, so we should probably rename /xhr/stats/ to something more relevant.

@iafan
Copy link
Contributor Author

iafan commented Nov 1, 2016

Agree on renaming this to browsing data.

julen added a commit to julen/zing that referenced this issue Nov 3, 2016
Previously, cached data from Redis would be used as-is in views. This was the
case both in the browsing view and the internal API endpoint to retrieve stats,
which would be consumed by the client code directly.

With the recent changes in the browsing pages, we changed the shape of the data
used by the client, but we didn't adjust the internal asynchronous endpoint that
retrieves updated stats when these are dirty.

This commit not only fixes that inconsistency, but also enables the endpoint to
return browsing-related data, not only stats. This data will be the same as the
data provided in templates' context.

Fixes serge-community#62.
@julen julen closed this as completed in #74 Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants