search

ooni / explorer

OONI Explorer: uncover evidence of internet censorship worldwide
https://explorer.ooni.org
BSD 3-Clause "New" or "Revised" License
71 stars 37 forks source link

New network centric view #750

Closed majakomel closed 2 years ago

majakomel commented 2 years ago

Closes https://github.com/ooni/explorer/issues/744

vercel[bot] commented 2 years ago

@majakomel is attempting to deploy a commit to the OONI Team on Vercel.

A member of the Team first needs to authorize it.

majakomel commented 2 years ago

@hellais This is a rough implementation of the network view - it needs some more refactoring/cleanup and changes to the date selector when the other ticket is ready to be merged.

Is there any clear idea yet what to put on the top of the page in the "high-level stats" area? I can start working on that part too.

hellais commented 2 years ago

Thanks for putting this together. I left in the original issue some ideas for what we can do with the high level stats: https://github.com/ooni/explorer/issues/744#issuecomment-1108467317.

Re: this branch, here is some feedback:

  1. Because of how the API works, asking for AS1234 is the same as asking for 1234. I think on these pages, though, we should enforce stricter checks on what is an allowed ASN (we should ensure it starts with AS). Not doing so will lead to two pages having the same content which, amongst other things, is bad for SEO.
  2. We should have a better page for when a network does not exist. I would say we can detect when we have no data for a particular network and display something like "This network either does not exist or we have no data for it"
  3. The AS number should be displayed prominently at the top of the page
  4. There is something odd going on with the web connectivity chart. The scrollbar is not visible. I recall us discussing this in the past with @sarathms and I think support for setting the scrollbar as visible was added.
majakomel commented 2 years ago

@hellais That for the feedback and review, although this is just a first draft and there are several parts to still be refactored. I'll add the fixes based on the feedback + more meaningful info to the top of the page and let you know when it's ready for review.

majakomel commented 2 years ago

@hellais I refactored all charts into one component and extracted two fetchers. Should I add a TODO comment or is there a way to add a follow-up ticket when backend adds support for multiple test_names?

Few notes:

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
explorer ✅ Ready (Inspect) Visit Preview May 30, 2022 at 3:01PM (UTC)
hellais commented 2 years ago

This is looking good.

Some comments based on an inspection of the preview:

1.

Screenshot 2022-05-16 at 16 26 39

In the measurement overview screen I think we should color code the dates in the future differently, since it's expected for there to not be any data there. I would suggest using an even lighter grey for those dates.

2.

For the dates which have zero data, it would be good if we had in those cases also a tooltip that shows 0 in it, so it's clear that is what is going on here.

3.

For the "Network observed in countries: XXX", I think we should make the XXX country name a link to the relevant country page so that we connect these two pages together.

4. When I access a network for which we don't have any data, I think we should have a specific 404 page or have a zero state for that network page. We can display something similar to when we have no OONI data for a certain country encouraging people to run measurements: https://explorer.ooni.org/country/AQ.

5.

I think the URLs for the networks should start with AS{NUMBER}, this gives us the flexibility to re-route endpoints based on the prefix if we ever decide to change the format (maybe we want to put the network name in the address bar instead of the ASN).

Sorry if my initial comment on this point wasn't clear.

Answering your questions in the above comment:

Should I add a TODO comment or is there a way to add a follow-up ticket when backend adds support for multiple test_names?

I would say we should document it in an ooni/explorer issue and link it to the backend one so we remember to do it once there is backend support.

For now I didn't add custom ranges - the colors are distributed based on how the library works by default (calculated based on min/max of the whole data range). There seems to be the functionality to define a custom range, but it's not documented so I have to yet figure it out.

I think it would be good if we could figure out how to get the ranges to work properly. The issue with using ranges that are relative to a given network is that it will make it hard to compare charts of different networks (for example what is colored green in one network, might be colored red in another, because they have big spikes of many more measurements).

Can you give me some more background on the scrollbar issue on the web connectivity chart? Currently it looks the same as on MAT page, but maybe I'm missing something.

I tested this again with the latest deployed version and I don't seem to be able to reproduce this anymore.

hellais commented 2 years ago

Another thing that would be good to have, is if we could sort the list of "Network observed in countries" by the number of measurements and perhaps also display the total number of measurements for each country.

This is useful because it would allow us to spot cases in which the network is wrongly geolocated to a specific country.

majakomel commented 2 years ago

I addressed all the points from the comments, color range is also set now. Empty network page only shows CTA - let me know if you want to add more content.

hellais commented 2 years ago

I think this looks good and we should merge and deploy it as is.

We should document some items to be done for future work, which can be implemented iteratively as follow up PRs: