Fix implementation of repo Home func

[Morlinest]

No description provided.

[lunny]

Seems it will never be models.UnitCode.URI

[Morlinest]

It shouldn't, but to be sure we don't have dead loop.

[codecov-io]

Codecov Report

Merging #2601 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2601      +/-   ##
==========================================
- Coverage    27.3%   27.29%   -0.01%     
==========================================
  Files          86       86              
  Lines       17139    17144       +5     
==========================================
  Hits         4680     4680              
- Misses      11781    11786       +5     
  Partials      678      678

Impacted Files	Coverage Δ
models/unit.go	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bae9cbc...bd98c34. Read the comment docs.

[Morlinest]

I see 1 problem here... If the first unit in slice is external issue/wiki, we will be redirected to it. This is actual problem my colleague found few days ago.

[daviian]

I don't see the point in changing this.
If we need sorting anyway Units[0] is always UnitCode, except the user has no permission to UnitCode.
So your for loop either runs 1 time (if UnitCode is existing) or runs through all units and never goes into the if statement.

[Morlinest]

@daviian But you shouldn't care about sorting in this function... You can sort it as u wish, but I know only about having 1 slice of available units, nothing else.

[Morlinest]

Fixed problem with redirecting to external issue/wiki if there is another option.

[daviian]

As I said: Doesn't ensure that leftmost tab is opened.

[Morlinest]

@daviian You are right. This fix is not about that. Logic for that can be added in another PR.

[Morlinest]

@daviian With your #2603 it should do the magic. Units will be sorted, double checked UniCode existence and also skipping unnecessary redirects to external tracker or wiki.

[ethantkoenig]

Why is the unit.URI != models.UnitCode.URI condition necessary? From what I can tell, there is no way firstAvailable can be models.UnitTypeCode at this point

[Morlinest]

To avoid redirecting to the same page (causing an infinite loop). Safer method would be to check requested uri vs computed uri....

[lafriks]

@Morlinest but that can't happen as only unit with same url is code and if it will be allowed it will render and return and won't get to this point

[Morlinest]

@lafriks It shouldn't, now, but... There can also be new unit with same uri like Issue and ExternalTracker or Wiki and ExternalWiki. You know what's in code, that function doesn't. I don't think making safe functions is wrong.

[Morlinest]

Actually I've just finished even more secure implementation. It can be used for each unit handler.

[Morlinest]

@ethantkoenig @lunny @ethantkoenig Removed uri check...

[ethantkoenig]

LGTM

[Morlinest]

I've changed implementation to use Unit.idx to determine first available RepoUnit for redirect if UnitTypeCode is missing. With this change, #2603 will not be necessary and #2621 can be changed to remove RepoUnit.Index completly. If you don't like my use of struct, I have simpler version with map[string]*models.Unit, but for price of losing type safety (as using custom type and constants for key would be probably an overkill). What do you think?

/cc @ethantkoenig @lafriks @daviian

Edit: Add type safety to map version means adding 5 lines of code.

[ethantkoenig]

Using double pointers and internal structs seems unnecessarily complicated. Can we please just do something like the following:

if ctx.Repo.Repository.UnitEnabled(models.UnitTypeCode) {
	renderCode(ctx)
	return
}
redirectUnitType = ctx.Repo.Repository.Units[0].Type
for _, unit := range ctx.Repo.Repository.Units {
	// LessThan is a helper method to add to unit type, we can 
	// implement it so that it favors non-external units
	if unit.Type.LessThan(redirectUnitType) {
		redirectUnitType = unit.Type
	}
}
redirectUnit := models.Units[redirectUnitType]
ctx.Redirect(fmt.Sprintf("%s/%s%s", setting.AppSubURL,
	ctx.Repo.Repository.FullName(),
	redirectUnit.URI))

[Morlinest]

OK,I'll use map version instead. The reason for not using only 1 variable is to avoid redirecting to external tracker or wiki if there is another (internal) tab available.

Edit: Read your solution again, it would be better. I've made it too complicated

[Morlinest]

@ethantkoenig @daviian Done

[daviian]

@Morlinest Please add my changed fixture file from #2603 for testing.

[Morlinest]

@daviian Done

[lafriks]

LGTM