open-contracting / data-registry

BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

Search page remembers filters, clear selection doesn't cancel all filters #219

Closed jpmckinney closed 2 years ago

jpmckinney commented 2 years ago

I can only see 8 datasets until I open a private window. Viktor can't see any.

I am not signed into the website.

yolile commented 2 years ago

The list of datasets is generated as a computed method https://github.com/open-contracting/data-registry/blob/f7e84a75ab97f1f1c492d63b47be9b98139cbd01/data_registry/vue/js/main.js#L310, we can move that method to methods instead so the list is not cached, see https://vuejs.org/guide/essentials/computed.html#computed-caching-vs-methods (as generating that list is not expensive)

jpmckinney commented 2 years ago

Perfect, thank you for debugging!

yolile commented 2 years ago

Reopening as there are still some cache issues

jpmckinney commented 2 years ago

The HTML contains <script src="/static/20220629Jun1656541276/js/main.js"></script> so it looks like there is already a cache-busting strategy for the JS file (putting a timestamp in the filepath, so that the browser doesn’t treat it as the same main.js file as was last retrieved). I’m not sure at what level the list of datasets is cached…

If there is any caching being done at the level of Django, it should be easy to turn it off via Django settings.

jakubkrafka commented 2 years ago

Hi James, sorry, I don't know your current deployment strategy and don't have access to the server at all. Also, it's not clear to me, whats the current caching problem. Therefore I cannot help precisely enough. So, at least the guide - you are right with the javascript (and other static files) caching principle. Its based on variable in the settings https://github.com/open-contracting/data-registry/blob/f8a4f08bacb8701a41cbc2f11c6d7a7d617c4e89/core/settings.py#L144 Currently defined as ENV variable. In general, such variable should be changed each time the js changes/rebuilds. The value itself is ignored by the rewrite rules, server each time serves the same file -> This will force reload on user side during next page visit. Sorry, its hard for me to help more - if above doesn't help, don't hesitate to reach me on Slack and we can solve this together.

EDIT: @jpmckinney mentioning you here as I'm not sure about your internal processes and whether this comment will reach you. Sorry, that it took me a week to realize:)

jpmckinney commented 2 years ago

I can no longer reproduce by visiting the page - perhaps my cache expired. Anyhow:

So, the data being delivered from the server is all correct.

However, I found new ways to reproduce the same/similar issue, which might have been responsible for this issue all along (but it's not caching related).

  1. If I set some filters, and then return to the page, the same filters are set. This might be surprising, as there is no indication of filters in the URL (e.g. query string parameters). If I come back to the page after a month, I might not notice that filters are set. (If the page had a very clear design with respect to active filters, then maybe not having query string parameters would be OK). The filters should be reset whenever the page is loaded (note: back button should still work).
  2. If I set the letter filter while on the Russian page, switching to English causes 0 results to be returned. The filters should be reset when changing language.
  3. "Clear selection" clears the letter filter, but it doesn't seem to clear other filters (I haven't tested them all). The link should clear all filters.
jpmckinney commented 2 years ago

Okay, well (3) is easy: the button calls setCountryFilter(), which only clears the letter filter when passed no arguments.

Fixed in 89fd232

https://github.com/open-contracting/data-registry/blob/64e790cba6876dfe81d0e2fd844e13b5a36adc87/data_registry/templates/search.html#L29-L35

jpmckinney commented 2 years ago

I think (1) and (2) are fixed by ce5872924cef6640002b872207b9c7a5542a1631. If we wanted to leave (1) as-is, then we'd need to implement a new solution for (2) (basically - check that the countryFilter value is valid in the current language).

jpmckinney commented 2 years ago

Hmm, I feel like I've barely changed anything (https://github.com/open-contracting/data-registry/compare/64e790cba6876dfe81d0e2fd844e13b5a36adc87..cfb18b55f13a5e34005b43cee512a4753ba163f0), but now I see no datasets at all. I don't even get an error...

I'd be happy to remove the store entirely, but there's logic on the dataset detail page to limit the date range for the Excel export to the same as on the search page. Honestly, I don't know what's wrong with old school query string parameters – this problem wouldn't even exist with those basic approaches, instead of abusing browser local storage.

yolile commented 2 years ago

Honestly, I don't know what's wrong with old school query string parameters – this problem wouldn't even exist with those basic approaches, instead of abusing browser local storage.

I think that for this particular project, the issue is that we are mixing Django templates with Vue apps. We have 3 different Vue apps (one for the search page, one for the detail page, and one for the header. In regular Vue projects, you only have one app for the entire website, so no local storage is needed. We are using a local store here only because we have 3 different apps, and the only way to share info between them is using local storage. And why are we using 3 different apps? Because we have to deal with the Django templates that work differently.

And for stage management, we use Vuex, but also this library https://github.dev/open-contracting/data-registry/blob/cfb18b55f13a5e34005b43cee512a4753ba163f0/data_registry/vue/js/main.js#L9 vuex-persistedstate that is used for the store data to be persistent even if the page is reloaded, etc.

yolile commented 2 years ago

This is a helpful blog where I think our issues are described

jpmckinney commented 2 years ago

Hmm, query string parameters would also work fine for sharing info between pages/apps (this.$route.query) and across page reloads. This would also make it visible to a knowledgeable user, AND if you load the page without any parameters (like visiting the registry after a month), then the query string parameters will be absent and you won't see filtered results.

yolile commented 2 years ago

Hmm, query string parameters would also work fine for sharing info between pages/apps

Yes, but we will need to manually change/set the query parameters each time a filter is applied, right? (and then, in the details page, we need to get the parameters from the URL) And we need to test how that interaction with Django urls works. I mean, I'm in favor of using query parameters, I'm only looking for the reasons behind this approach.

jpmckinney commented 2 years ago

Local storage also means that, if I open another window and set different filters, then my two windows will interact with each other through the local storage, which is totally unexpected.

Reading/writing query string parameters is not more complex that reading/writing local storage – it's just a different (and more appropriate) way of communicating state.

I don't think the Django views are reading the query string, so should have no effect.

jpmckinney commented 2 years ago

Vue 2 is end of life 5 months ago. https://endoflife.date/vue

jpmckinney commented 2 years ago

I'm cleaning up weird code. I have no idea why this is done. Just having a regular HTML link with a download attribute would do the same thing. I don't see any reason to "pipe" the GZIP response through JavaScript... It literally creates an a tag whose URL is the file's data (!), clicks the link, then removes the link...

https://github.com/open-contracting/data-registry/blob/cfb18b55f13a5e34005b43cee512a4753ba163f0/data_registry/vue/js/main.js#L448-L477

yolile commented 2 years ago

🤷🏻‍♀️

but now I see no datasets at all. I don't even get an error...

The same occurs when deploying locally, so I'm trying to see which commit affected the search list behavior so silently and badly.

jpmckinney commented 2 years ago

As an update, I just need to finish fixing the styles on the details page. Then I can work on converting the search page.

So far just need 8 lines of JS. The download form is a plain HTML form.