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

Show added reviewer name #7669

Open
mschnitzer opened this issue Jun 4, 2019 · 4 comments
Open

Show added reviewer name #7669

mschnitzer opened this issue Jun 4, 2019 · 4 comments
Labels
Feature Frontend Things related to the OBS RoR app

Comments

@mschnitzer
Copy link
Member

Is your feature request related to a problem? Please describe.
When adding a new review, the name of the reviewer is not shown in the history. It would make sense to see who was added.

Describe the solution you'd like
The history element tells you who was added.

Describe alternatives you've considered
n/a

Additional context
It looks like that at the moment:

review

@dmarcoux dmarcoux added Feature Frontend Things related to the OBS RoR app labels Jun 4, 2019
@dmarcoux
Copy link
Contributor

@danidoni: As you can see in the screenshot above, we have this text:

mschnitzer added a review 6 minutes ago

It doesn't specify who the reviewer is. It should instead be:

mschnitzer added a review for reviewer_name 6 minutes ago

@coolo
Copy link
Member

coolo commented Sep 11, 2019

@danidoni
Copy link
Contributor

Note that there are 4 different review types:
https://github.com/openSUSE/open-build-service/pull/7591/files#diff-75675f82fdd365004e71101ff8d9fbefR140

Oh, thanks for pointing this out. I'll have it in mind.

@danidoni
Copy link
Contributor

We found a dead end here:

The Request history box gets a collection of HistoryElements. To show the reviewer for each item in the collection, we have to fetch the review from the HistoryElement, then we handle it to the _request_history_element.html.haml partial which knows how to render the different types of reviewers.

This introduces an N+1 performance issue while acquiring the reviewer for each and every history element.

To try to fix this, we added a belongs_to association from HistoryElement::RequestReviewAdded to its Review, so we could, later, eager load it via .includes(:review) from the inverse association, the BsRequest#request_history_elements. But it didn't work, as BsRequest#request_history_elements iterates over all HistoryElement types, but only HistoryElements::RequestReviewAdded is capable of accessing its review association via its description_extension field. The rest of the types don't have the association, so it breaks when we try to resolve the association.

Adding this association for all types don't work, as it doesn't have any meaning for the rest of the types (its description_extension field does not contain an id pointing to its review, but something else, so it won't join correctly).

Another approach would be to split the BsRequest#history_elements association into an association for each of its subclasses, then apply the eager load in the ones that would work, and somehow join them later for the Request history page (and some others) to work as before. But this is a hack. It has several side effects as we'll have to UNION the rows returned somehow, and its a pretty ugly solution after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants