tf / redmine_merge_request_links

Display links to associated Gitlab merge requests and GitHub pull requests on Redmine's issue page.
https://www.redmine.org/plugins/redmine_merge_request_links
MIT License
35 stars 26 forks source link

Search issues by associated merge requests #31

Closed jpraet closed 3 years ago

jpraet commented 3 years ago

Fixes #29

Adds "Merge requests" as available column for issue queries.

image

You can also filter by merge request status:

test

jpraet commented 3 years ago

Regarding design, the new column looks a bit crowded for my taste. I was wondering if it might make sense to only display counts of open/merged/closed PRs in the index table or go for a more condensed representation of MRs with titles only being displayed as tooltips. What do you think?

I like to be able to navigate from this view directly to the PR. That wouldn't be possible if we only display the count. The PR representation is already condensed to only show the identifier and status. The PR title is already in a tooltip as it is: image

But I agree it does look a bit crowded. Maybe just show the #id? And group them by status somehow?

Or put each MR on a new line?

image

tf commented 3 years ago

I was looking at the built in "Related issues" column and it pretty much takes the same approach as your initial proposal. What do you think about replacing the open/merged/closed badges with small indicators - little circles or boxes with only a color and the actual state name as tooltip/title? This might make the table just as easy to scan without taking up all the space. Another thought I had (here comes the feature creep :wink:), was introducing additional columns only for open or closed merge requests. Depending on the use case this might also be helpful and limit the amount of items visible.

jpraet commented 3 years ago

Something like this with status icons maybe? image

jpraet commented 3 years ago

Well.. I tried adding the tests but they are failing on 4.0. I don't know why.. I did some manual testing with 3.4, 4.0 and 4.1 and it works fine.

tf commented 3 years ago

The test failure is due to your usage of params in the tests. To make the tests work for both Redmine 3 (Rails 4) and Redmine 4 (Rails 5), we patch the request test helpers to wrap the params in a params hash. Since your tests already use the params keyword from the start, the params get wrapped once to often. Redmine::ControllerTest, that you switched to, provides compat in the other direction making your tests pass for Redmine 3. It would probably be best to switch to your solution throughout and remove the custom test helper.

tf commented 3 years ago

Regarding design: I like the icons in general. One thing I do not like is the visual discrepancy that it creates between the table and the show view since the icons currently are not used in the merge request list on the issue page.

Another idea that might be a compromise: Use badges that resemble the open/merged/closed badges on the show page, but use single letters instead:

Bildschirmfoto von 2021-02-01 10-50-32

If you strongly prefer the icons, I'm also fine with taking that route.

tf commented 3 years ago

Thank you for your contribution! :tada: