Show depending and blocked PRs in the PR list

[zokkis]

trafficstars

Show depending and blocked PRs/Issues in the PR/Issue list (Resolves #29018) and added a divider between texts in the item-body

before:

after:

[silverwind]

I would lowercase the text. All other text on the line is lowercase, so it looks out of place to title-case.

[zokkis]

Two things I noticed while testing:

1. We should swap the order of `blocks`/`depends on` around, as the `depends on` is much more important in most cases, so should be the first info you see.

2. There's no clear separator when both are present, making the display slightly confusing. Not a blocker.

Apart from that, there might be some performance problems when querying a repo with many PRs/ a large dependency network. However, I don't know if they are (easily) fixable. Otherwise LGTM.

I fixed it with a "|" between the list-items

[Greg-NetDuma]

This is my subjective opinion as someone who made the feature request, items in priority order (this PR might already do some of them in that case ignore those points):

• If the dependency is resolved (closed I guess) I don't want to see it in the depends_on list.
• The PR refs (#1) should be clickable
• I don't care if a PR blocks and for me it's not necessary to display it in the list, I only care if a PR is blocked since one way is enough to work out the dependency tree, and depends_on directly affects when that PR might get reviewed while blocks affects the other PRs instead.
• depends on is a bit soft wording because effectively that PR is blocked by other PR's and I would prefer that if said PR would indicate the blocked state more prominently. You could
  • Change the wording to blocked by (might be too similar to blocks)
  • Change the icon of the PR as well
• You might need to handle a big number of dependencies differently in the frontend, we sometimes have PRs with 5-10 dependencies. One option is that with 3+ dependencies you could just display #1, #2, #3 and more, but I'm not confident of a solution here.

With the exception of the first point if the things above sound unreasonable or they might not align with the majority of the people I'm ok with the current state of this PR.

[zokkis]

This is my subjective opinion as someone who made the feature request, items in priority order (this PR might already do some of them in that case ignore those points):

* If the dependency is resolved (closed I guess) I don't want to see it in the `depends_on` list.

* The PR refs (`#1`) should be clickable

* I don't care if a PR `blocks` and for me it's not necessary to display it in the list, I only care if a PR is blocked.

* `depends on` is a bit soft wording because effectively that PR is blocked by other PR's and I would prefer that if said PR would indicate the blocked state more prominently. You could
  
  * Change the wording to `blocked by` (might be too similar to `blocks`)
  * Change the icon of the PR as well

* You might need to handle a big number of dependencies differently in the frontend, we sometimes have PRs with 5-10 dependencies. One option is that with 3+ dependencies you could just display `#1, #2, #3 and more`, but I'm not confident of a solution here.

With the exception of the first point if the things above sound unreasonable or they might not align with the majority of the people I'm ok with the current state of this PR.

1. Good idea, I will try to implement it
2. all links are clickable
3. You don't need this feature, but its now in the list and maybe someone needs it
4. 'depends on' is the official wording
5. Yes its correct, a large number ob dependencies is a bit ugly in the list, but how often is the number of dependencies so large? And when I do #1, #2, #3 and more its no longer possible to click the link directly. I will look into it, but currently I don't have a good solution.

[silverwind]

UI looks good. Only thing that maybe could be improved is to manually emit the separators in HTML instead of CSS because I fear that the CSS selector may have false matches, but at least from the screenshots, it looks alright.

[zokkis]

UI looks good. Only thing that maybe could be improved is to manually emit the separators in HTML instead of CSS because I fear that the CSS selector may have false matches, but at least from the screenshots, it looks alright.

Its a bit repetitive to add everywhere the <div class="gt-font-18">&middot;</div> so I added a helper-class I hope its better now

[silverwind]

Fine with me in any case.

[6543]

3110bd349a2c176553f303a1fe4541310a660f7c

thanks :)

[6543]

Could you make it a setting to be deactivated ...?

... this is rather costly as it triggers a lof more sql querys for a single view

[zokkis]

The settings are used in services/doctor/fix16961.go did this file need a change?

[lunny]

It's better to have a test for this since it has obvious un-tested parts.

[zokkis]

It's better to have a test for this since it has obvious un-tested parts.

I think you mean the new functions BlockingDependenciesMap and BlockedByDependenciesMap The problem is, that I never wrote any go tests and I looked a bit into it and don't know how to add a good test, because I need new fixures for IssueDepencency and a good idea on how to properly test this functions

[lunny]

It's better to have a test for this since it has obvious un-tested parts.

I think you mean the new functions BlockingDependenciesMap and BlockedByDependenciesMap

The problem is, that I never wrote any go tests and I looked a bit into it and don't know how to add a good test, because I need new fixures for IssueDepencency and a good idea on how to properly test this functions

If you don't mind, I think I can help to write the tests.

[zokkis]

It's better to have a test for this since it has obvious un-tested parts.

I think you mean the new functions BlockingDependenciesMap and BlockedByDependenciesMap The problem is, that I never wrote any go tests and I looked a bit into it and don't know how to add a good test, because I need new fixures for IssueDepencency and a good idea on how to properly test this functions

If you don't mind, I think I can help to write the tests.

Thanks good to hear this. Do you write the test or do I need to write the test and you explain me how to do this?

[6543]

I would sugest to write unit tests. the correct location for your tests should then be models/issues/issue_list_test.go.

For tests we have "fixtures" who represent the initial database state for each test: https://github.com/go-gitea/gitea/tree/main/models/fixtures

[6543]

PS: just look at some existing tests, you mostly can replicate things and adjust it to your needs

[zokkis]

@lunny @6543 I hope this test is ok

[6543]

yes the your commit itselfe looks fine, the implications (as we use the fixtures for all tests ...) well have to be addressed.

I'll have a look and push if i can fix it easily, else I'll write my findings here ;)

[zokkis]

yes the your commit itselfe looks fine, the implications (as we use the fixtures for all tests ...) well have to be addressed.

I'll have a look and push if i can fix it easily, else I'll write my findings here ;)

Is it maybe better to use issues_model.CreateIssueDependency instead of a fixture?

[silverwind]

Needs a merge in the migration files, then I will test it.

[zokkis]

Needs a merge in the migration files, then I will test it.

I will wait for the last review from lunny, then I will resolve conflicts

[silverwind]

Needs a merge in the migration files, then I will test it.

I will wait for the last review from lunny, then I will resolve conflicts

Ok, but give me some time as well then to do a quick UI test 😆.

[lunny]

Sorry I made the PR broken so I have to force push. And I have done a local test, looks like the blocked issue/pr will not displayed in the pr list.

[zokkis]

Sorry I made the PR broken so I have to force push. And I have done a local test, looks like the blocked issue/pr will not displayed in the pr list.

Did you enabled the setting?

[silverwind]

Tried it but does not seem to work:

Issue 3 has 2 dependencies, one of them being outside the repo, the other in same repo:

[zokkis]

Its working, but you have to enable the setting

Maybe make a clean clone? Because I did a new clean clone and it worked

[silverwind]

Ah, it works now after enabling that. I hadn't expected it to be locked behind a setting.

[zokkis]

Ah, it works now after enabling that. I hadn't expected it to be locked behind a setting.

Its because of https://github.com/go-gitea/gitea/pull/29117#issuecomment-1949939868

[silverwind]

I think the rendering could be made similar to milestone, with an icon before an no text besides issue numbers joined by ", ". The question is just which icons to use for "depends on" and "blocks". I don't think octicons has anything suitable, but https://www.svgrepo.com/ might.

I noticed the dot we added causes unsatisfactory rendering for link hover. We need to fix that, maybe just remove the dot after all. A bit of whitespace should be enough between items.

[silverwind]

These two are pretty good already, We could switch the box to a rounded outlined square:

https://primer.github.io/octicons/package-dependencies-16 https://primer.github.io/octicons/package-dependents-16

There is also a fitting icon for a "issue blocks":

https://primer.github.io/octicons/issue-tracked-by-16

The idea is that if the arrow in the icon indicates the direction of the dependency, pointing from icon to the numbers for "depends on" and from the numbers to icon for "blocks".