Prevent 500 is head repo does not have PullRequest unit in IsUserAllo…

[zeripath]

…wedToUpdate

Some repositories do not have the PullRequest unit present in their configuration
and unfortunately the way that IsUserAllowedToUpdate currently works assumes
that this is an error instead of just returning false.

This PR simply swallows this error allowing the function to return false.

Fix #20621

Signed-off-by: Andrew Thornton art27@cantab.net

[wolfbeast]

This PR simply swallows this error allowing the function to return false.

Wouldn't this cause confusion for merging if it's "not allowed to be merged" by returning false when the fork repo/PR source is having this error, that could be solved by making any change to the PR settings in the fork that makes the entries? The one normally doing the merging would not have access to the fork repo to know this...

[zeripath]

This PR simply swallows this error allowing the function to return false.

Wouldn't this cause confusion for merging if it's "not allowed to be merged" by returning false when the fork repo/PR source is having this error, that could be solved by making any change to the PR settings in the fork that makes the entries? The one normally doing the merging would not have access to the fork repo to know this...

This PR is simply stopping the 500 and is easily backportable to 1.17.x

What you are describing is new UI handling which will require new translatable strings and so on. That will not be backportable to 1.17 without significant work.

Propose the UI and describe a reproducing testcase in another issue - but it is out of scope for this PR.

[wolfbeast]

What you are describing is new UI handling which will require new translatable strings and so on. That will not be backportable to 1.17 without significant work.

Yes understood :) I know l10n is the ever-looming issue when touching the UI or error messages (first hand experience).
I was just wondering if it was considered. Personally, I think making sure the db has the correct entries when migrating to an affected version would avoid having to do UI work altogether and could be a good follow-up, but I don't know how you normally handle something like this so I'll let you decide what direction to go here.
Sorry for the tangent.

[codecov-commenter]

Codecov Report

Merging #20839 (1803e6a) into main (03df7d0) will increase coverage by 0.32%.
The diff coverage is 0.00%.

❗ Current head 1803e6a differs from pull request most recent head c0192bb. Consider uploading reports for the commit c0192bb to get more accurate results

@@            Coverage Diff             @@
##             main   #20839      +/-   ##
==========================================
+ Coverage   46.74%   47.07%   +0.32%     
==========================================
  Files         989      989              
  Lines      136393   136395       +2     
==========================================
+ Hits        63761    64202     +441     
+ Misses      64778    64310     -468     
- Partials     7854     7883      +29     

Impacted Files	Coverage Δ
services/pull/update.go	46.15% <0.00%> (-0.81%)	⬇️
modules/indexer/stats/indexer.go	51.06% <0.00%> (-6.39%)	⬇️
modules/notification/mail/mail.go	39.04% <0.00%> (-2.74%)	⬇️
modules/web/routing/funcinfo.go	91.89% <0.00%> (-2.71%)	⬇️
modules/notification/ui/ui.go	58.92% <0.00%> (-1.79%)	⬇️
modules/queue/queue_disk_channel.go	68.33% <0.00%> (-1.25%)	⬇️
models/notification.go	62.22% <0.00%> (-1.10%)	⬇️
models/issues/comment.go	50.43% <0.00%> (+0.43%)	⬆️
services/pull/pull.go	40.80% <0.00%> (+0.46%)	⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.