Add approval entry UI to try results
Change-Id: Ibc3c14bac1b53de2a1c77fc5ec21e3c8630421cc
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/126520
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/.gitignore b/.gitignore
index 0faa90f..98b9306 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,3 +11,4 @@
functions/node_modules
github-label-notifier/functions/node_modules
results_feed/results_feed.iml
+results_feed/test/page_objects/*.g.dart
diff --git a/results_feed/lib/src/components/app_component.dart b/results_feed/lib/src/components/app_component.dart
index e3fc2d2..575bdc9 100644
--- a/results_feed/lib/src/components/app_component.dart
+++ b/results_feed/lib/src/components/app_component.dart
@@ -271,7 +271,7 @@
AppComponentTest(this.appComponent);
- StagingFirestoreService get firestoreService =>
+ TestingFirestoreService get firestoreService =>
appComponent._firestoreService;
ApplicationRef get applicationRef => appComponent._applicationRef;
}
diff --git a/results_feed/lib/src/components/blamelist_component.html b/results_feed/lib/src/components/blamelist_component.html
index 2c83999..b900c21 100644
--- a/results_feed/lib/src/components/blamelist_component.html
+++ b/results_feed/lib/src/components/blamelist_component.html
@@ -13,8 +13,9 @@
</div>
</div>
<div *ngFor="let c of comments" class="comment">
- <b>{{c.approvedText()}}</b> {{formattedEmail(c.author)}}
- <span class="nowrap">{{formattedDate(c.created)}}</span><br>
+ <b>{{c.approvedText()}}</b>
+ <span class="nowrap">{{formattedDate(c.created)}}</span>
+ {{formattedEmail(c.author)}}<br>
<div class="comment-body">{{c.comment}}</div>
</div>
</div>
diff --git a/results_feed/lib/src/components/blamelist_picker.html b/results_feed/lib/src/components/blamelist_picker.html
index f821658..b953576 100644
--- a/results_feed/lib/src/components/blamelist_picker.html
+++ b/results_feed/lib/src/components/blamelist_picker.html
@@ -12,8 +12,8 @@
</material-radio>
<div *ngFor="let c of comments" class="comment">
<b>{{c.approvedText()}}</b>
- {{formattedEmail(c.author)}}
<span class="nowrap">{{formattedDate(c.created)}}</span><br>
+ {{formattedEmail(c.author)}}
<div class="comment-body">{{c.comment}}</div>
</div>
</material-radio-group>
diff --git a/results_feed/lib/src/components/commit_component.dart b/results_feed/lib/src/components/commit_component.dart
index 0804900..4b076b6 100644
--- a/results_feed/lib/src/components/commit_component.dart
+++ b/results_feed/lib/src/components/commit_component.dart
@@ -73,12 +73,12 @@
return firestoreService.saveApproval(
approval,
commentText,
- [for (Change result in selected) result.id],
changeGroup.comments.isEmpty
? null
: changeGroup.comments.last.baseComment ??
changeGroup.comments.last.id,
- changeGroup.range.start,
- changeGroup.range.end);
+ resultIds: [for (Change result in selected) result.id],
+ blamelistStart: changeGroup.range.start,
+ blamelistEnd: changeGroup.range.end);
}
}
diff --git a/results_feed/lib/src/components/results_selector_panel.html b/results_feed/lib/src/components/results_selector_panel.html
index 0e25759..762ba52 100644
--- a/results_feed/lib/src/components/results_selector_panel.html
+++ b/results_feed/lib/src/components/results_selector_panel.html
@@ -53,6 +53,7 @@
{{approvalContent(change)}}{{change.name}}
</span><br>
<material-tooltip-card
+ *ngIf="range.length > 0"
[for]="logs"
[preferredPositions]="preferredTooltipPositions">
<div *deferredContent>
diff --git a/results_feed/lib/src/components/try_results_component.dart b/results_feed/lib/src/components/try_results_component.dart
index 19b242b..09de21d 100644
--- a/results_feed/lib/src/components/try_results_component.dart
+++ b/results_feed/lib/src/components/try_results_component.dart
@@ -3,20 +3,37 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:angular/angular.dart';
+import 'package:angular_components/angular_components.dart';
+import 'package:angular_components/material_button/material_button.dart';
+import 'package:angular_components/material_input/material_input.dart';
+import 'package:angular_components/material_input/material_input_multiline.dart';
+import 'package:angular_forms/angular_forms.dart' show formDirectives;
import 'package:angular_router/angular_router.dart';
+import '../formatting.dart';
import '../model/comment.dart';
import '../model/commit.dart';
-import 'results_panel.dart';
import '../services/try_data_service.dart';
+import 'results_panel.dart';
+import 'results_selector_panel.dart';
@Component(
selector: 'try-results',
- directives: [coreDirectives, routerDirectives, ResultsPanel],
+ directives: [
+ coreDirectives,
+ formDirectives,
+ materialInputDirectives,
+ MaterialButtonComponent,
+ MaterialMultilineInputComponent,
+ ResultsPanel,
+ ResultsSelectorPanel
+ ],
providers: [
ClassProvider(TryDataService),
+ overlayBindings,
],
templateUrl: 'try_results_component.html',
+ exports: [formattedDate, formattedEmail],
styles: ['div {padding: 8px;}'])
class TryResultsComponent implements OnActivate {
TryDataService _tryDataService;
@@ -33,9 +50,41 @@
IntRange range = IntRange(1, 0);
bool updating = false;
bool updatePending = false;
+ bool _approving = false;
+ final selected = Set<Change>();
+ String commentText;
TryResultsComponent(this._tryDataService, this._applicationRef);
+ bool get approveEnabled => changeGroup.changes.flat
+ .any((change) => change.result != change.expected);
+
+ bool get approving => _approving;
+ set approving(bool approve) {
+ if (approve) {
+ _tryDataService.logIn().then((_) {
+ _approving = _tryDataService.isLoggedIn;
+ });
+ } else {
+ _approving = false;
+ }
+ }
+
+ Future approve(bool approval) {
+ for (Change result in selected) {
+ result.approved = approval ?? result.approved;
+ }
+ return _tryDataService.saveApproval(
+ approval,
+ commentText,
+ changeGroup.comments.isEmpty
+ ? null
+ : changeGroup.comments.last.baseComment ??
+ changeGroup.comments.last.id,
+ [for (Change result in selected) result.id],
+ changeInfo.review);
+ }
+
void tryUpdate() async {
if (updating) {
updatePending = true;
diff --git a/results_feed/lib/src/components/try_results_component.html b/results_feed/lib/src/components/try_results_component.html
index 8681726..9c56399 100644
--- a/results_feed/lib/src/components/try_results_component.html
+++ b/results_feed/lib/src/components/try_results_component.html
@@ -5,13 +5,63 @@
</h2>
<h2 *ngIf="changeGroup.changes.isEmpty">No changed test results</h2>
- <results-panel
+ <results-panel *ngIf="!approving"
[changes]="changeGroup.changes"
[range]="range">
</results-panel>
+ <results-selector-panel *ngIf="approving"
+ [changes]="changeGroup.changes"
+ [range]="range"
+ [selected]="selected"
+ failuresOnly>
+ </results-selector-panel>
<div *ngFor="let c of changeGroup.comments">
- <b>{{c.approvedText()}}</b> {{c.author}} {{formatted(c.created)}}<br>
+ <b>{{c.approvedText()}}</b>
+ <span class="nowrap">{{formattedDate(c.created)}}</span>
+ {{formattedEmail(c.author)}}<br>
<div style="padding-left: 32px">{{c.comment}}</div>
</div>
+ <div *ngIf="approving">
+ Comment:<br>
+ <material-input
+ multiline
+ style="width: 80%"
+ label="Comment"
+ required
+ rows="3"
+ [(ngModel)]="commentText"></material-input>
+ </div>
+ <div>
+ <material-button
+ *ngIf="approveEnabled && !approving"
+ raised
+ (click)="approving = true">
+ Approve/Comment ...
+ </material-button>
+ <material-button
+ *ngIf="approving"
+ raised
+ (click)="approving = false">
+ Cancel
+ </material-button>
+ <material-button
+ *ngIf="approving"
+ raised
+ (click)="approve(false)">
+ Revoke Selected Approvals
+ </material-button>
+ <material-button
+ *ngIf="approving"
+ raised
+ (click)="approve(null)">
+ Comment without Approving
+ </material-button>
+ <material-button
+ *ngIf="approving"
+ raised
+ (click)="approve(true)">
+ Approve
+ </material-button>
+ </div>
</div>
\ No newline at end of file
diff --git a/results_feed/lib/src/services/firestore_service.dart b/results_feed/lib/src/services/firestore_service.dart
index 5daf670..1a3882d 100644
--- a/results_feed/lib/src/services/firestore_service.dart
+++ b/results_feed/lib/src/services/firestore_service.dart
@@ -135,29 +135,41 @@
}
}
- Future saveApproval(bool approve, String comment, List<String> resultIds,
- String baseComment, blamelistStart, blamelistEnd) async {
- app.firestore().collection('comments').add({
+ Future saveApproval(bool approve, String comment, String baseComment,
+ {List<String> resultIds,
+ List<String> tryResultIds,
+ int blamelistStart,
+ int blamelistEnd,
+ int review}) async {
+ await app.firestore().collection('comments').add({
'author': app.auth().currentUser.email,
if (comment != null) 'comment': comment,
'created': DateTime.now(),
if (approve != null) 'approved': approve,
- 'results': resultIds,
- 'blamelist_start_index': blamelistStart,
- 'blamelist_end_index': blamelistEnd
+ if (resultIds != null) 'results': resultIds,
+ if (tryResultIds != null) 'try_results': tryResultIds,
+ if (blamelistStart != null) 'blamelist_start_index': blamelistStart,
+ if (blamelistStart != null) 'blamelist_end_index': blamelistEnd,
+ if (review != null) 'gerrit_change': review
});
- // Update approved field in results documents.
if (approve == null) return;
- // If resultIds.length > 500, break into multiple batches.
- for (final results in iterables.partition(resultIds, 500)) {
- final batch = app.firestore().batch();
- final collection = app.firestore().collection('results');
- for (final id in results) {
- batch
- .update(collection.doc(id), fieldsAndValues: ['approved', approve]);
+ // Update approved field in results documents.
+ Future<void> approveResults(List<String> ids, String collectionName) async {
+ if (ids == null) return;
+ // If resultIds.length > 500, break into multiple batches.
+ for (final results in iterables.partition(ids, 500)) {
+ final batch = app.firestore().batch();
+ final collection = app.firestore().collection(collectionName);
+ for (final id in results) {
+ batch.update(collection.doc(id),
+ fieldsAndValues: ['approved', approve]);
+ }
+ await batch.commit();
}
- await batch.commit();
}
+
+ await approveResults(resultIds, 'results');
+ await approveResults(tryResultIds, 'try_results');
}
Future<void> getFirebaseClient() async {
@@ -177,9 +189,6 @@
}
class StagingFirestoreService extends FirestoreService {
- // StagingFirestoreService uses the Firestore database at
- // dart-ci-staging.
- // It adds additional methods used by integration tests.
Future<void> getFirebaseClient() async {
if (app != null) return;
// Firebase api key is public, and must be sent to client for use.
@@ -193,9 +202,34 @@
projectId: "dart-ci-staging",
storageBucket: "dart-ci-staging.appspot.com");
await app.auth().setPersistence(firebase.Persistence.LOCAL);
+ }
+}
+
+class TestingFirestoreService extends StagingFirestoreService {
+ // TestingFirestoreService uses the Firestore database at
+ // dart-ci-staging.
+ // It adds additional methods used by integration tests.
+ // It includes local password-based authentication for
+ // tests that write to staging.
+ Future<void> getFirebaseClient() async {
+ await super.getFirebaseClient();
await logIn();
}
+ Future logIn() async {
+ try {
+ await app.auth().signInWithEmailAndPassword(
+ 'dartresultsfeedtestuser@example.com', r'');
+ // Password must be entered locally before testing.
+ // Because this is running in a browser, cannot read password from
+ // a file or environment variable. Investigate what testing framework
+ // supports for local secrets.
+ } catch (e) {
+ print(e.toString());
+ }
+ return;
+ }
+
Future<void> writeDocumentsFrom(Map<String, dynamic> documents,
{bool delete = false}) async {
for (final keys in iterables.partition(documents.keys, 500)) {
@@ -226,18 +260,4 @@
await batch.commit();
}
}
-
- Future logIn() async {
- try {
- await app.auth().signInWithEmailAndPassword(
- 'dartresultsfeedtestuser@example.com', r'');
- // Password must be entered locally before testing.
- // Because this is running in a browser, cannot read password from
- // a file or environment variable. Investigate what testing framework
- // supports for local secrets.
- } catch (e) {
- print(e.toString());
- }
- return;
- }
}
diff --git a/results_feed/lib/src/services/try_data_service.dart b/results_feed/lib/src/services/try_data_service.dart
index 30dd346..e74c1fa 100644
--- a/results_feed/lib/src/services/try_data_service.dart
+++ b/results_feed/lib/src/services/try_data_service.dart
@@ -8,6 +8,9 @@
final FirestoreService _firestoreService;
TryDataService(this._firestoreService);
+ Future logIn() => _firestoreService.logIn();
+ bool get isLoggedIn => _firestoreService.isLoggedIn;
+
Future<List<Change>> changes(ReviewInfo changeInfo, int patch) async {
final patchsets = changeInfo.patchsets;
if (patchsets.length < patch) return [];
@@ -39,6 +42,11 @@
final docs = await _firestoreService.fetchCommentsForReview(review);
return [for (final doc in docs) Comment.fromDocument(doc)];
}
+
+ Future saveApproval(bool approve, String comment, String baseComment,
+ Iterable<String> resultIds, int review) =>
+ _firestoreService.saveApproval(approve, comment, baseComment,
+ tryResultIds: resultIds, review: review);
}
class ReviewInfo {
diff --git a/results_feed/test/comments_test.dart b/results_feed/test/comments_test.dart
index 5b2b8e7..8970e5e 100644
--- a/results_feed/test/comments_test.dart
+++ b/results_feed/test/comments_test.dart
@@ -21,7 +21,7 @@
// pub run build_runner test --fail-on-severe -- -p chrome comments_test.dart
@GenerateInjector([
- ClassProvider(FirestoreService, useClass: StagingFirestoreService),
+ ClassProvider(FirestoreService, useClass: TestingFirestoreService),
])
final InjectorFactory rootInjector = self.rootInjector$Injector;
diff --git a/results_feed/web/main.dart b/results_feed/web/main.dart
index 906d70e..bceb6d7 100644
--- a/results_feed/web/main.dart
+++ b/results_feed/web/main.dart
@@ -9,7 +9,8 @@
as ng;
import 'main.template.dart' as self;
-// Local testing use @GenerateInjector(routerProvidersHash)
+// Local testing use
+/// @GenerateInjector([ClassProvider(FirestoreService, useClass: TestingFirestoreService), ...routerProvidersHash])
// Use for deploying on staging website:
// @GenerateInjector([ClassProvider(FirestoreService, useClass: StagingFirestoreService), ...routerProviders])
