1564833 - [meta] Server side search in Alerts view

Status: NEW

(Tree Management :: Perfherder, enhancement, P1)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Attachment: Unnecessary multiple search page results.mp4

When searching for a specific word in Alerts page of Perfherder, it doesn't collect the results down to one page if they fit. Video with steps attached.

This bug is making Perfsheriffs' process of identifying regressions more difficult as it requires some additional clicks.

Comment 1

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Sarah, I'm waiting for further guidance on this. Thanks!

Comment 2

[Alexandru Ionescu (needinfo me) [:alexandrui]]

And because this is an improving perfherder task, I would also pagination at the top of the page, not only bottom.

Comment 3

[Sarah Clements [:sclements]]

(In reply to Alexandru Ionescu :alexandrui from comment #2)

And because this is an improving perfherder task, I would also pagination at the top of the page, not only bottom.

It'll be easier to review if you can keep the two tasks in separate commits and possibly two separate pr's.

Have you gotten yourself set up with docker? If not, you'll need to get that set up to work on the backend: https://treeherder.readthedocs.io/installation.html#server-and-full-stack-development

I suggest filing a bug in the Treeherder component to get access to the Treeherder-production-replica RDS instance (read only) so that you will have ample data for testing out API changes locally.

We use React/Javascript in the UI and Django/python in the backend so you'll need to have familiarity with both (for Django, understanding the queryset API should be sufficient). We use django-rest-framework with Django to structure our APIs.

I think what you'll what to do is look into passing a query parameter with the search term from the search input to the API that is called to fetch data, so the top 10 results will be returned based on that filter (we want to keep the existing pagination since new data is called when a user navigates to a new page) . You'll also probably need to change the search input component or create a new one that is used only for the alerts view so that it fetches data, not just update the state.

This is where you'll want to start to familiarize yourself with the pagination code: https://github.com/mozilla/treeherder/blob/master/ui/perfherder/alerts/AlertsView.jsx#L268

This is where the search input component is used: https://github.com/mozilla/treeherder/blob/master/ui/perfherder/alerts/AlertsViewControls.jsx#L54

Adding pagination to the top would probably be the easiest to start with. :)

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Thank you, Sarah. Looks like a quite big task. A good occasion to get familiarized with js stuff. I'll file another bug to add pagination to the top.
This will not be top priority, but I'll work on it between the priority tasks.
Dave, you ok with this?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We already have a fair amount of Perfherder work planned for Q3, so I'm curious to know if this should be higher priority than the dependencies on bug 1563748.

Comment 6

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Probably this depends on the working style of the sheriff. I'm still in the ramp-up phase of learning perfherder, so I use fairly often the search function. But I think this function is useful anytime you want to search through all the alerts of certain type (raptor, awsy, etc).
It's true that requires quite high effort from someone outside of the perfherder development team. Probably should be set for next/future quarter or so.
Marian, bebe?

Comment 7

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Sarah, I thrown a closer eye on your guidance above and I think this is a pretty complicated task for who's not actively working on that. Also it includes opening a bug for permissions. As this is not on the priority list for me&my team, probably I should wait until someone more suitable for this will take care. I appreciate that you made time to write that guidance steps.
Thanks for fixing the lack of persistence of the search term. It is really helpful for at least my work on sheriffing.
Should I assign this to you or leave it unassigned?

Comment 8

[Sarah Clements [:sclements]]

I don't think I'll have time to work on it so you can mark it as a P3 and leave it unassigned (we're aiming to only mark something as P1 or P2 if it's assigned to someone).

Comment 9

[Florin Strugariu [:Bebe]]

This would be useful for sure. But as it's not in the current priority list I agree with the P3

Comment 11

[Suyash Salampuria]

Since the first task(adding pagination to the top) is already done,
I wish to proceed with the 2nd task

I think what you'll what to do is look into passing a query parameter with the search term from the search input to the API that is called to fetch data, so the top 10 results will be returned based on that filter (we want to keep the existing pagination since new data is called when a user navigates to a new page) . You'll also probably need to change the search input component or create a new one that is used only for the alerts view so that it fetches data, not just update the state.

What I understood from the video was that on searching anything in the filterText input column, it gives results in from that page only,
but we desire results from the whole group of pages.
I followed the input value till
updateFilterText={filterText => this.setState({ filterText })}
in AlertViewControls.jsx
I would like to follow it and solve this
Could you please guide me in this and correct me if I am wrong in understanding this
Thanks in advance!!

Comment 12

[Sarah Clements [:sclements]]

From my understanding some other people are working on the tasks assigned to this meta bug (so this meta bug should be marked as assigned). Right Ionut?

Comment 13

[Ionuț Goldan [:igoldan]]

(In reply to Sarah Clements [:sclements] from comment #12)

From my understanding some other people are working on the tasks assigned to this meta bug (so this meta bug should be marked as assigned). Right Ionut?

Yes, that's right. Thanks for looking over this.