-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feat: drop origin from issue listing #1105
Conversation
1b7a38c
to
de22fd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much slower but worked well on my tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that @murilx added the culprit_code filter in the frontend, but I believe that with the query being as slow as it got now, it might be better to move that filter to the backend query, because with your current change the backend is trying to get the first_seen value for 297 issues but it will only show the 172 issues with culprit_code == true in the frontend. Filtering in the backend via an optional query parameter and defaulting it to culprit_code in the frontend would lower the request time while keeping the option to get all culprits via the api if needed.
de22fd1
to
c3708c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add tests for the new query parameters
1238d90
to
6fdfbf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working well, but you should update the requests file related to this change. I believe it's issue-listing-get.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked well on my tests, I also think the request file should be updated, but I'll approve already
6fdfbf9
to
9b5f921
Compare
- add tests to culprit params Part of #1075
9b5f921
to
d2665b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked well on my tests
@@ -1,4 +1,4 @@ | |||
http 'localhost:8000/api/issue/?origin=maestro&intervalInDays=1' | |||
http 'localhost:8000/api/issue/?&intervalInDays=1&culpritCode=true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not necessarily need to come with culpritCode
but I like the addition
Close #1075
How to test