Add overview and reviewers information

[rubhanazeem]

Preview of the changes: For reviewers:

• Log in as Iggy / buildservice
• Visit https://obs-reviewlab.opensuse.org/rubhanazeem-request-overview/request/show/2
• If you don't see the request show redesign, the feature toggle request_show_redesign needs to enabled for the staff group in the Flipper UI
• Visit again the request#show page and you should see the redesign now

DO NOT MERGE until we squash the commits.

[rubhanazeem]

I think we can do better than displaying the reviewers exactly like we did before. Maybe showing who approved/declined the requests and who still needs to review.

This information is already available in the request history. I've removed the duplicate request state from overview tab

[hennevogel]

This information is already available in the request history.

But not in a form that I can get an overview what is done and what not.

Also the repeated text (Open review for...) is a headline and I think you should display the by_project reviews for projects that belong to a staging differently.

[dmarcoux]

As @rubhanazeem and I discussed together this morning, we need to have a better overview on who approved/declined the request and who still needs to review the request (if a request is still in review).

Here's a mockup of a proposal, displaying with icons the reviewers and their review state. This would be the content of the Overview tab. The request description can be quite long in some cases, so I wouldn't display the reviewers below the description.

[obs-bot]

Review app will appear here: http://obs-reviewlab.opensuse.org/rubhanazeem-request-overview

[saraycp]

@rubhanazeem I'd recommend rebasing on master as soon as possible, so we get rid of code already handled by PR #12858. It will make this easier to review.

[hennevogel]

BTW visually you can review this by going here: https://obs-reviewlab.opensuse.org/rubhanazeem-request-overview/request/beta/show/2

[codecov[bot]]

Codecov Report

Merging #12863 (4289c15) into master (64b8ac5) will decrease coverage by 0.06%. The diff coverage is 41.93%.

:exclamation: Current head 4289c15 differs from pull request most recent head d77e522. Consider uploading reports for the commit d77e522 to get more accurate results

@@            Coverage Diff             @@
##           master   #12863      +/-   ##
==========================================
- Coverage   88.69%   88.63%   -0.07%     
==========================================
  Files         694      695       +1     
  Lines       23721    23752      +31     
==========================================
+ Hits        21039    21052      +13     
- Misses       2682     2700      +18     

[saraycp]

Please, DO NOT MERGE until we squash the commits.

[dmarcoux]

To help users understand who exactly is involved in reviews, we should display the name of the users in addition to their login. So as an example with the screenshot below, Admin would become Jane Doe (Admin). This change affects the reviewer for groups/projects/packages reviews and the reviews for a single user.

[saraycp]

we should display the name of the users in addition to their login

@rubhanazeem also suggested that. I'll do it.

[saraycp]

To help users understand who exactly is involved in reviews, we should display the name of the users in addition to their login. So as an example with the screenshot below, Admin would become Jane Doe (Admin). This change affects the reviewer for groups/projects/packages reviews and the reviews for a single user.

If we use the long version of the name, this is what it looks like:

We think it's not nice when the name is too long. We vote for using the login only. If the user hovers over the avatar, he/she will see the real name. Moreover, the link we display goes to the profile, so revealing the real name is just one click away.

[saraycp]

RSpec is failing because we introduced the method Review#by_package?. Now I'm trying to figure out why...

[hennevogel]

I think the conversation needs a headline on smaller viewports or the reviews need to go somewhere else...

[saraycp]

RSpec is failing because we introduced the method Review#by_package?. Now I'm trying to figure out why...

We added a new commit fixing this issue. Please read the commit description for more details.

[hennevogel]

We added a new commit fixing this issue. Please read the commit description for more details.

So we are back to being implicit with ordering. I don't like it particularly. You can name those methods without clashing with rails right?

[rubhanazeem]

I think the conversation needs a headline on smaller viewports or the reviews need to go somewhere else...

The heading was missing due to merge conflicts. It's back now

[saraycp]

We added a new commit fixing this issue. Please read the commit description for more details.

So we are back to being implicit with ordering. I don't like it particularly. You can name those methods without clashing with rails right?

We also thought about renaming the methods... we've just pushed a commit doing so now. Naming is always tricky, we hope requested_to_*? (requested_to_project?) is ok.

[saraycp]

I added a tiny change and rebased on master.

[saraycp]

@dmarcoux and @hennevogel , we have rebased on master and squashed all the commits in one. Please take another look.