Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit d9116de

Browse files
grokysgrokys
authored
Merge pull request #1059 from github/fixes/1049-surface-pr-errors
Surface errors when loading PR details.
2 parents d210d20 + 28dce3f commit d9116de

File tree

7 files changed

+192
-84
lines changed

7 files changed

+192
-84
lines changed

‎src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ public PullRequestDetailViewModelDesigner()
8686
public IPullRequestCheckoutState CheckoutState { get; set; }
8787
public IPullRequestUpdateState UpdateState { get; set; }
8888
public string OperationError { get; set; }
89+
public string ErrorMessage { get; set; }
8990

9091
public ReactiveCommand<Unit> Checkout { get; }
9192
public ReactiveCommand<Unit> Pull { get; }

‎src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs‎

Lines changed: 94 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDet
3939
IReadOnlyList<IPullRequestChangeNode> changedFilesTree;
4040
IPullRequestCheckoutState checkoutState;
4141
IPullRequestUpdateState updateState;
42+
string errorMessage;
4243
string operationError;
4344
bool isBusy;
4445
bool isLoading;
@@ -236,6 +237,15 @@ public IPullRequestUpdateState UpdateState
236237
private set { this.RaiseAndSetIfChanged(ref updateState, value); }
237238
}
238239

240+
/// <summary>
241+
/// Gets an error message to display if loading fails.
242+
/// </summary>
243+
public string ErrorMessage
244+
{
245+
get { return errorMessage; }
246+
private set { this.RaiseAndSetIfChanged(ref errorMessage, value); }
247+
}
248+
239249
/// <summary>
240250
/// Gets the error message to be displayed in the action area as a result of an error in a
241251
/// git operation.
@@ -298,10 +308,16 @@ public override void Initialize([AllowNull] ViewWithData data)
298308
else
299309
IsBusy = true;
300310

301-
OperationError = null;
311+
ErrorMessage = OperationError = null;
302312
modelService.GetPullRequest(Repository, prNumber)
303313
.TakeLast(1)
304314
.ObserveOn(RxApp.MainThreadScheduler)
315+
.Catch<IPullRequestModel, Exception>(ex =>
316+
{
317+
ErrorMessage = ex.Message.Trim();
318+
IsLoading = IsBusy = false;
319+
return Observable.Empty<IPullRequestModel>();
320+
})
305321
.Subscribe(x => Load(x).Forget());
306322
}
307323

@@ -312,94 +328,103 @@ public override void Initialize([AllowNull] ViewWithData data)
312328
/// <param name="files">The pull request's changed files.</param>
313329
public async Task Load(IPullRequestModel pullRequest)
314330
{
315-
var firstLoad = (Model == null);
316-
Model = pullRequest;
317-
Session = await sessionManager.GetSession(pullRequest);
318-
Title = Resources.PullRequestNavigationItemText + " #" + pullRequest.Number;
319-
320-
IsFromFork = pullRequestsService.IsPullRequestFromFork(Repository, Model);
321-
SourceBranchDisplayName = GetBranchDisplayName(IsFromFork, pullRequest.Head?.Label);
322-
TargetBranchDisplayName = GetBranchDisplayName(IsFromFork, pullRequest.Base.Label);
323-
CommentCount = pullRequest.Comments.Count + pullRequest.ReviewComments.Count;
324-
Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;
331+
try
332+
{
333+
var firstLoad = (Model == null);
334+
Model = pullRequest;
335+
Session = await sessionManager.GetSession(pullRequest);
336+
Title = Resources.PullRequestNavigationItemText + " #" + pullRequest.Number;
325337

326-
var changes = await pullRequestsService.GetTreeChanges(Repository, pullRequest);
327-
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
338+
IsFromFork = pullRequestsService.IsPullRequestFromFork(Repository, Model);
339+
SourceBranchDisplayName = GetBranchDisplayName(IsFromFork, pullRequest.Head?.Label);
340+
TargetBranchDisplayName = GetBranchDisplayName(IsFromFork, pullRequest.Base.Label);
341+
CommentCount = pullRequest.Comments.Count + pullRequest.ReviewComments.Count;
342+
Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown;
328343

329-
var localBranches = await pullRequestsService.GetLocalBranches(Repository, pullRequest).ToList();
344+
var changes = await pullRequestsService.GetTreeChanges(Repository, pullRequest);
345+
ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList();
330346

331-
IsCheckedOut = localBranches.Contains(Repository.CurrentBranch);
347+
var localBranches = await pullRequestsService.GetLocalBranches(Repository, pullRequest).ToList();
332348

333-
if (IsCheckedOut)
334-
{
335-
var divergence = await pullRequestsService.CalculateHistoryDivergence(Repository, Model.Number);
336-
var pullEnabled = divergence.BehindBy > 0;
337-
var pushEnabled = divergence.AheadBy > 0 && !pullEnabled;
338-
string pullToolTip;
339-
string pushToolTip;
349+
IsCheckedOut = localBranches.Contains(Repository.CurrentBranch);
340350

341-
if (pullEnabled)
342-
{
343-
pullToolTip = string.Format(
344-
Resources.PullRequestDetailsPullToolTip,
345-
IsFromFork ? Resources.Fork : Resources.Remote,
346-
SourceBranchDisplayName);
347-
}
348-
else
351+
if (IsCheckedOut)
349352
{
350-
pullToolTip = Resources.NoCommitsToPull;
351-
}
353+
var divergence = await pullRequestsService.CalculateHistoryDivergence(Repository, Model.Number);
354+
var pullEnabled = divergence.BehindBy > 0;
355+
var pushEnabled = divergence.AheadBy > 0 && !pullEnabled;
356+
string pullToolTip;
357+
string pushToolTip;
352358

353-
if (pushEnabled)
354-
{
355-
pushToolTip = string.Format(
356-
Resources.PullRequestDetailsPushToolTip,
357-
IsFromFork ? Resources.Fork : Resources.Remote,
358-
SourceBranchDisplayName);
359-
}
360-
else if (divergence.AheadBy == 0)
361-
{
362-
pushToolTip = Resources.NoCommitsToPush;
359+
if (pullEnabled)
360+
{
361+
pullToolTip = string.Format(
362+
Resources.PullRequestDetailsPullToolTip,
363+
IsFromFork ? Resources.Fork : Resources.Remote,
364+
SourceBranchDisplayName);
365+
}
366+
else
367+
{
368+
pullToolTip = Resources.NoCommitsToPull;
369+
}
370+
371+
if (pushEnabled)
372+
{
373+
pushToolTip = string.Format(
374+
Resources.PullRequestDetailsPushToolTip,
375+
IsFromFork ? Resources.Fork : Resources.Remote,
376+
SourceBranchDisplayName);
377+
}
378+
else if (divergence.AheadBy == 0)
379+
{
380+
pushToolTip = Resources.NoCommitsToPush;
381+
}
382+
else
383+
{
384+
pushToolTip = Resources.MustPullBeforePush;
385+
}
386+
387+
UpdateState = new UpdateCommandState(divergence, pullEnabled, pushEnabled, pullToolTip, pushToolTip);
388+
CheckoutState = null;
363389
}
364390
else
365391
{
366-
pushToolTip = Resources.MustPullBeforePush;
367-
}
392+
var caption = localBranches.Count > 0 ?
393+
string.Format(Resources.PullRequestDetailsCheckout, localBranches.First().DisplayName) :
394+
string.Format(Resources.PullRequestDetailsCheckoutTo, await pullRequestsService.GetDefaultLocalBranchName(Repository, Model.Number, Model.Title));
395+
var clean = await pullRequestsService.IsWorkingDirectoryClean(Repository);
396+
string disabled = null;
368397

369-
UpdateState = new UpdateCommandState(divergence, pullEnabled, pushEnabled, pullToolTip, pushToolTip);
370-
CheckoutState = null;
371-
}
372-
else
373-
{
374-
var caption = localBranches.Count > 0 ?
375-
string.Format(Resources.PullRequestDetailsCheckout, localBranches.First().DisplayName) :
376-
string.Format(Resources.PullRequestDetailsCheckoutTo, await pullRequestsService.GetDefaultLocalBranchName(Repository, Model.Number, Model.Title));
377-
var clean = await pullRequestsService.IsWorkingDirectoryClean(Repository);
378-
string disabled = null;
398+
if (pullRequest.Head == null || !pullRequest.Head.RepositoryCloneUrl.IsValidUri)
399+
{
400+
disabled = Resources.SourceRepositoryNoLongerAvailable;
401+
}
402+
else if (!clean)
403+
{
404+
disabled = Resources.WorkingDirectoryHasUncommittedCHanges;
405+
}
379406

380-
if (pullRequest.Head == null || !pullRequest.Head.RepositoryCloneUrl.IsValidUri)
381-
{
382-
disabled = Resources.SourceRepositoryNoLongerAvailable;
407+
CheckoutState = new CheckoutCommandState(caption, disabled);
408+
UpdateState = null;
383409
}
384-
else if (!clean)
410+
411+
if (firstLoad)
385412
{
386-
disabled = Resources.WorkingDirectoryHasUncommittedCHanges;
413+
usageTracker.IncrementPullRequestOpened().Forget();
387414
}
388415

389-
CheckoutState = new CheckoutCommandState(caption, disabled);
390-
UpdateState = null;
416+
if (!isInCheckout)
417+
{
418+
pullRequestsService.RemoveUnusedRemotes(Repository).Subscribe(_ => { });
419+
}
391420
}
392-
393-
IsLoading = IsBusy = false;
394-
395-
if (firstLoad)
421+
catch (Exception ex)
396422
{
397-
usageTracker.IncrementPullRequestOpened().Forget();
423+
ErrorMessage = ex.Message.Trim();
398424
}
399-
400-
if (!isInCheckout)
425+
finally
401426
{
402-
pullRequestsService.RemoveUnusedRemotes(Repository).Subscribe(_ => { });
427+
IsLoading = IsBusy = false;
403428
}
404429
}
405430

‎src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public interface IPullRequestUpdateState
6464
/// <summary>
6565
/// Represents a view model for displaying details of a pull request.
6666
/// </summary>
67-
public interface IPullRequestDetailViewModel : IViewModel, IHasLoading, IHasBusy
67+
public interface IPullRequestDetailViewModel : IViewModel, IHasLoading, IHasBusy, IHasErrorState
6868
{
6969
/// <summary>
7070
/// Gets the underlying pull request model.

‎src/GitHub.Exports/GitHub.Exports.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@
137137
<Compile Include="Models\ICommentModel.cs" />
138138
<Compile Include="Models\IInlineCommentModel.cs" />
139139
<Compile Include="Models\IPullRequestReviewCommentModel.cs" />
140+
<Compile Include="ViewModels\IHasErrorState.cs" />
140141
<Compile Include="ViewModels\IHasLoading.cs" />
141142
<Compile Include="ViewModels\IPanePageViewModel.cs" />
142143
<Compile Include="ViewModels\IViewModel.cs" />

‎src/GitHub.Exports/ViewModels/IHasErrorState.cs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
using System;
2+
3+
namespace GitHub.ViewModels
4+
{
5+
/// <summary>
6+
/// Interface for view models that have an error state.
7+
/// </summary>
8+
public interface IHasErrorState
9+
{
10+
/// <summary>
11+
/// Gets the view model's error message or null if the view model is not in an error state.
12+
/// </summary>
13+
string ErrorMessage { get; }
14+
}
15+
}

‎src/GitHub.UI.Reactive/Controls/ViewBase.cs‎

Lines changed: 58 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,16 @@ namespace GitHub.UI
2121
/// </remarks>
2222
public class ViewBase : ContentControl
2323
{
24-
static readonly DependencyPropertyKey HasBusyStatePropertyKey =
24+
static readonly DependencyPropertyKey ErrorMessagePropertyKey =
2525
DependencyProperty.RegisterReadOnly(
26-
nameof(HasBusyState),
26+
nameof(ErrorMessage),
27+
typeof(string),
28+
typeof(ViewBase),
29+
new FrameworkPropertyMetadata());
30+
31+
static readonly DependencyPropertyKey HasStatePropertyKey =
32+
DependencyProperty.RegisterReadOnly(
33+
nameof(HasState),
2734
typeof(bool),
2835
typeof(ViewBase),
2936
new FrameworkPropertyMetadata());
@@ -42,16 +49,25 @@ public class ViewBase : ContentControl
4249
typeof(ViewBase),
4350
new FrameworkPropertyMetadata());
4451

52+
static readonly DependencyPropertyKey ShowContentPropertyKey =
53+
DependencyProperty.RegisterReadOnly(
54+
nameof(ShowContent),
55+
typeof(bool),
56+
typeof(ViewBase),
57+
new FrameworkPropertyMetadata());
58+
4559
static readonly DependencyProperty ShowBusyStateProperty =
4660
DependencyProperty.Register(
4761
nameof(ShowBusyState),
4862
typeof(bool),
4963
typeof(ViewBase),
5064
new FrameworkPropertyMetadata(true));
5165

52-
public static readonly DependencyProperty HasBusyStateProperty = HasBusyStatePropertyKey.DependencyProperty;
66+
public static readonly DependencyProperty ErrorMessageProperty = ErrorMessagePropertyKey.DependencyProperty;
67+
public static readonly DependencyProperty HasStateProperty = HasStatePropertyKey.DependencyProperty;
5368
public static readonly DependencyProperty IsBusyProperty = IsBusyPropertyKey.DependencyProperty;
5469
public static readonly DependencyProperty IsLoadingProperty = IsLoadingPropertyKey.DependencyProperty;
70+
public static readonly DependencyProperty ShowContentProperty = ShowContentPropertyKey.DependencyProperty;
5571

5672
static ViewBase()
5773
{
@@ -62,14 +78,24 @@ static ViewBase()
6278
VerticalContentAlignmentProperty.OverrideMetadata(typeof(ViewBase), new FrameworkPropertyMetadata(VerticalAlignment.Stretch));
6379
}
6480

81+
/// <summary>
82+
/// Gets a value reflecting the associated view model's <see cref="IHasErrorState.ErrorMessage"/> property.
83+
/// </summary>
84+
[AllowNull]
85+
public string ErrorMessage
86+
{
87+
get { return (string)GetValue(ErrorMessageProperty); }
88+
protected set { SetValue(ErrorMessagePropertyKey, value); }
89+
}
90+
6591
/// <summary>
6692
/// Gets a value indicating whether the associated view model implements <see cref="IHasLoading"/>
67-
/// or <see cref="IHasBusy"/>.
93+
/// <see cref="IHasBusy"/> or <see cref="IHasErrorState"/>.
6894
/// </summary>
69-
public bool HasBusyState
95+
public bool HasState
7096
{
71-
get { return (bool)GetValue(HasBusyStateProperty); }
72-
protected set { SetValue(HasBusyStatePropertyKey, value); }
97+
get { return (bool)GetValue(HasStateProperty); }
98+
protected set { SetValue(HasStatePropertyKey, value); }
7399
}
74100

75101
/// <summary>
@@ -99,6 +125,15 @@ public bool ShowBusyState
99125
set { SetValue(ShowBusyStateProperty, value); }
100126
}
101127

128+
/// <summary>
129+
/// Gets or sets a value indicating whether to the view model content.
130+
/// </summary>
131+
public bool ShowContent
132+
{
133+
get { return (bool)GetValue(ShowContentProperty); }
134+
protected set { SetValue(ShowContentPropertyKey, value); }
135+
}
136+
102137
internal ViewBase()
103138
{
104139
}
@@ -109,9 +144,9 @@ internal ViewBase()
109144
/// </summary>
110145
/// <remarks>
111146
/// Exposes a typed <see cref="ViewModel"/> property and optionally displays <see cref="IHasLoading.IsLoading"/>
112-
/// and <see cref="IHasBusy.IsBusy"/> state if the view model implements those interfaces. In addition, if the view
113-
/// model is an <see cref="IDialogViewModel"/>, invokes <see cref="IDialogViewModel.Cancel"/> when the escape key
114-
/// is pressed.
147+
/// <see cref="IHasBusy.IsBusy"/> and <see cref="IHasErrorState.ErrorMessage"/> state if the view model implements
148+
/// those interfaces. In addition, if the view model is an <see cref="IDialogViewModel"/>, invokes
149+
/// <see cref="IDialogViewModel.Cancel"/> when the escape key is pressed.
115150
/// </remarks>
116151
public class ViewBase<TInterface, TImplementor> : ViewBase, IView, IViewFor<TInterface>, IDisposable
117152
where TInterface : class, IViewModel
@@ -134,6 +169,7 @@ public ViewBase()
134169

135170
var hasLoading = ViewModel as IHasLoading;
136171
var hasBusy = ViewModel as IHasBusy;
172+
var hasErrorState = ViewModel as IHasErrorState;
137173
var subs = new CompositeDisposable();
138174

139175
if (hasLoading != null)
@@ -146,7 +182,12 @@ public ViewBase()
146182
subs.Add(this.OneWayBind(hasBusy, x => x.IsBusy, x => x.IsBusy));
147183
}
148184

149-
HasBusyState = hasLoading != null || hasBusy != null;
185+
if (hasErrorState != null)
186+
{
187+
subs.Add(this.OneWayBind(hasErrorState, x => x.ErrorMessage, x => x.ErrorMessage));
188+
}
189+
190+
HasState = hasLoading != null || hasBusy != null;
150191
subscriptions = subs;
151192
};
152193

@@ -161,6 +202,12 @@ public ViewBase()
161202
(this.ViewModel as IDialogViewModel)?.Cancel.Execute(null);
162203
}));
163204
});
205+
206+
this.WhenAnyValue(
207+
x => x.IsLoading,
208+
x => x.ErrorMessage,
209+
(l, m) => !l && m == null)
210+
.Subscribe(x => ShowContent = x);
164211
}
165212

166213
/// <summary>

0 commit comments

Comments
 (0)