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

Issue when copying a data portal URL query from one google chrome tab to another #1034

Closed
aclum opened this issue Aug 9, 2023 · 19 comments · Fixed by #1056 or #1149
Closed

Issue when copying a data portal URL query from one google chrome tab to another #1034

aclum opened this issue Aug 9, 2023 · 19 comments · Fixed by #1056 or #1149
Assignees

Comments

@aclum
Copy link
Contributor

aclum commented Aug 9, 2023

I discovered what I believe is a separate bug with query results when debugging the load balancing issues today.
When I run a query, say select the neon study in one search window (ie https://data.microbiomedata.org/?q=Ch4IABABGAMiFiJubWRjOnN0eS0xMS0zNHhqMTE1MCI=) I get the expected results in terms of number of samples. Expected number of active search results is 4204.
If I copy the url with the applied filter (https://data.microbiomedata.org/?q=Ch4IABABGAMiFiJubWRjOnN0eS0xMS0zNHhqMTE1MCI=) to a new tab in google chrome and press enter the active query search results starts as 0, this is expected as the search is loading, then it momentarily shows the correct result of 4204, then it updates to 6734 which is the total number of biosamples in the data portal. The incorrect results seems to be limited to number of samples and the active search query results. The upset plot, map, and omics bar chart filter correctly.
Screenshot 2023-08-08 at 10 26 05 PM

Chrome version Version 115.0.5790.170
This is reproducible in safari Version 16.1 (18614.2.9.1.12). The first time I copy the query into a safari tab the results are correct. Subsequent tabs produce the same incorrect behavior as chrome, sometimes skipping over monetarily having the correct 4204 value.

cc @mslarae13 @pkalita-lbl @shreddd @jeffbaumes

@aclum
Copy link
Contributor Author

aclum commented Aug 9, 2023

cc @ssarrafan

@jeffbaumes
Copy link
Collaborator

This is a race condition due to an initialization quirk. For some reason an initial unfiltered search is run on page load, and then the filtered search. In the case of your search, the unfiltered result returns after the filtered one, so there is a "last result wins" and the UI reflects this. I suspect the detailed sample search results are also incorrect in this case.

This image shows the timing, where you can see the second request is faster and returns first, but then will get replaced when the first request resolves.

image

One solution is to not run an unfiltered query at all when there is a URL-encoded query.

@jeffbaumes
Copy link
Collaborator

I suspect reordering the init function could help here, i.e. move the await api.me() line after the state.conditions = ... line:

state.user = await api.me();
router = _router;
// @ts-ignore
state.conditions = router.currentRoute.query?.conditions || [];

That way the query conditions are set right away before the async call to get the user, so that in the main startup the Vue app is initialized with the query conditions already set:

init(router);
new Vue({
router,
vuetify,
setup() {
provideRouter(router);
},
render: (h) => h(App),
}).$mount('#app');

@marySalvi or @naglepuff could you try this out and see if we get only one limit=15 sample query on loading a page with a query in the URL?

@ssarrafan
Copy link

Added to sprint late. Jeff said to add to new sprint at infra sync meeting today.

@ssarrafan
Copy link

ssarrafan commented Aug 25, 2023

@marySalvi is this issue actively being worked on?

@marySalvi
Copy link
Collaborator

Yes. Solution might be trickier than I originally thought. Please move to next sprint.

@ssarrafan
Copy link

Checked with @marySalvi and moving to next sprint.

@naglepuff
Copy link
Collaborator

The solution from #1056 ended up introducing some problems with other (non-search) API endpoints. We'll have to rework that to fix the race condition. In the meantime, that change has been reverted.

@naglepuff naglepuff reopened this Sep 28, 2023
@naglepuff naglepuff added the backlog Issues not assigned to a sprint or not finished during a sprint. Needs to be reprioritized. label Sep 28, 2023
@aclum
Copy link
Contributor Author

aclum commented Oct 24, 2023

Tentatively planned for the sprint the week of November 6

@aclum aclum removed the backlog Issues not assigned to a sprint or not finished during a sprint. Needs to be reprioritized. label Nov 3, 2023
@aclum
Copy link
Contributor Author

aclum commented Nov 3, 2023

Per infrastructure sync meeting @marySalvi will work on this ticket for the sprint starting Nov 6.

@aclum
Copy link
Contributor Author

aclum commented Nov 30, 2023

Moving to the next sprint per the infrastructure call.

@ssarrafan
Copy link

Last day of sprint. Removing from sprint. Adding backlog label.

@ssarrafan ssarrafan added the backlog Issues not assigned to a sprint or not finished during a sprint. Needs to be reprioritized. label Dec 15, 2023
@aclum aclum removed the backlog Issues not assigned to a sprint or not finished during a sprint. Needs to be reprioritized. label Jan 9, 2024
@shreddd
Copy link
Contributor

shreddd commented Jan 9, 2024

Hi Folks - we may need to bump this up in priority, since it actually gets triggered even outside of link sharing, if you try to login after a search.

To reproduce

  1. Go to https://data.microbiomedata.org/ without logging in
  2. Issue a search. In my simple example I just clicked on Metabolomics i.e. "Omics type [is] Metabolomics". This generates the a search URL i.e. https://data.microbiomedata.org/?q=ChYIABACGAIiDiJNZXRhYm9sb21pY3Mi
  3. Go through the login flow (When I try to download data, it says I need to log in, so I issued a login)
  4. Upon login it takes you back to the previous search URL but loses the search information.

So basically if one does a login after a search, it effectively triggers the bug.

@ssarrafan
Copy link

Moving to the new sprint for Mary.

@ssarrafan
Copy link

Discussed at infra sync meeting today. Will be reviewed by Mike this week by tomorrow.

@ssarrafan
Copy link

@naglepuff did you get a chance to review this?

@naglepuff
Copy link
Collaborator

@naglepuff did you get a chance to review this?

@ssarrafan yes

@aclum
Copy link
Contributor Author

aclum commented Mar 1, 2024

Confirmed this is working in dev and prod!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment