From 5cc027ea8407eb36a2d3f3bb792d7567a5de3bae Mon Sep 17 00:00:00 2001 From: Peter Chapman Date: Wed, 8 Jan 2025 12:51:23 +1300 Subject: [PATCH] Code review fixes --- .../event-metrics-log.component.spec.ts | 32 ++++++++++--- .../event-metrics-log.component.ts | 24 ++++++++-- .../Services/ISFProjectService.cs | 2 +- .../DataAccess/BsonValueConverter.cs | 1 + src/SIL.XForge/DataAccess/MemoryRepository.cs | 2 +- src/SIL.XForge/Services/EventMetricService.cs | 4 +- .../Services/EventMetricServiceTests.cs | 46 +++++++++++++++++++ 7 files changed, 96 insertions(+), 15 deletions(-) diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.spec.ts index 517649ee6b..087d466206 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.spec.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.spec.ts @@ -132,6 +132,25 @@ describe('EventMetricsLogComponent', () => { expect(env.nextButton.nativeElement.disabled).toBeTrue(); expect(env.rows.length).toEqual(2); })); + + it('should show custom event type calculations', fakeAsync(() => { + const env = new TestEnvironment(); + // This list is to ensure complete test coverage of the custom cases in EventMetricsLogComponent.getEventType() + let eventMetrics: Partial[] = [ + { eventType: 'SetIsValidAsync', payload: { isValid: true } }, + { eventType: 'SetIsValidAsync', payload: { isValid: false } }, + { eventType: 'SetDraftAppliedAsync', payload: { draftApplied: true } }, + { eventType: 'SetDraftAppliedAsync', payload: { draftApplied: false } }, + { eventType: 'SetPreTranslateAsync', payload: { preTranslate: true } }, + { eventType: 'SetPreTranslateAsync', payload: { preTranslate: false } } + ]; + + env.populateEventMetrics(eventMetrics); + env.wait(); + env.wait(); + + expect(env.rows.length).toEqual(eventMetrics.length); + })); }); class TestEnvironment { @@ -179,14 +198,15 @@ class TestEnvironment { this.wait(); } - populateEventMetrics(): void { + populateEventMetrics(eventMetrics: Partial[] | undefined = undefined): void { + eventMetrics ??= [ + { scope: EventScope.Sync, timeStamp: new Date().toISOString(), eventType: 'SyncAsync' } as EventMetric, + { scope: EventScope.Settings, timeStamp: new Date().toISOString(), eventType: 'UnknownType' } as EventMetric + ]; when(mockedProjectService.onlineEventMetrics(anything(), anything(), anything())).thenReturn( Promise.resolve({ - results: [ - { scope: EventScope.Sync, timeStamp: new Date().toISOString(), eventType: 'SyncAsync' } as EventMetric, - { scope: EventScope.Settings, timeStamp: new Date().toISOString(), eventType: 'UnknownType' } as EventMetric - ], - unpagedCount: 4 + results: eventMetrics, + unpagedCount: eventMetrics.length * 2 } as QueryResults) ); } diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts index d42f19a5ba..eb286ea98e 100644 --- a/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts +++ b/src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts @@ -37,7 +37,7 @@ export class EventMetricsLogComponent extends DataLoadingComponent implements On rows: Row[] = []; private pageIndex$ = new BehaviorSubject(0); - private pageSize$ = new BehaviorSubject(50); + private pageSize$ = new BehaviorSubject(10); length: number = 0; get pageIndex(): number { @@ -125,7 +125,7 @@ export class EventMetricsLogComponent extends DataLoadingComponent implements On for (const eventMetric of this.eventMetrics) { rows.push({ dialogData: eventMetric, - eventType: this.getEventType(eventMetric.eventType), + eventType: this.getEventType(eventMetric), scope: eventMetric.scope, successful: eventMetric.exception == null, timeStamp: this.i18n.formatDate(new Date(eventMetric.timeStamp), { showTimeZone: true }), @@ -135,7 +135,7 @@ export class EventMetricsLogComponent extends DataLoadingComponent implements On this.rows = rows; } - private getEventType(eventType: string): string { + private getEventType(eventMetric: EventMetric): string { // These values are the functions that have the LogEventMetric attribute, where: // - The case is the name of the method // - The return value is a user friendly description of what the method does @@ -153,6 +153,22 @@ export class EventMetricsLogComponent extends DataLoadingComponent implements On StartPreTranslationBuildAsync: 'Start draft generation', SyncAsync: 'Start synchronization with Paratext' }; - return eventTypeMap[eventType] || eventType; + + // Allow specific cases based on payload values + if (eventMetric.eventType === 'SetIsValidAsync' && eventMetric.payload['isValid'] === true) { + return 'Marked chapter as valid'; + } else if (eventMetric.eventType === 'SetIsValidAsync' && eventMetric.payload['isValid'] === false) { + return 'Marked chapter as invalid'; + } else if (eventMetric.eventType === 'SetDraftAppliedAsync' && eventMetric.payload['draftApplied'] === true) { + return 'Marked chapter as having draft applied'; + } else if (eventMetric.eventType === 'SetDraftAppliedAsync' && eventMetric.payload['draftApplied'] === false) { + return 'Marked chapter as not having a draft applied'; + } else if (eventMetric.eventType === 'SetPreTranslateAsync' && eventMetric.payload['preTranslate'] === true) { + return 'Set drafting as enabled for the project'; + } else if (eventMetric.eventType === 'SetPreTranslateAsync' && eventMetric.payload['preTranslate'] === false) { + return 'Set drafting as disabled for the project'; + } else { + return eventTypeMap[eventMetric.eventType] || eventMetric.eventType; + } } } diff --git a/src/SIL.XForge.Scripture/Services/ISFProjectService.cs b/src/SIL.XForge.Scripture/Services/ISFProjectService.cs index 67a7b54d82..06dfdffb76 100644 --- a/src/SIL.XForge.Scripture/Services/ISFProjectService.cs +++ b/src/SIL.XForge.Scripture/Services/ISFProjectService.cs @@ -81,7 +81,7 @@ string audioUrl Task SetDraftAppliedAsync(string userId, string projectId, int book, int chapter, bool draftApplied); [LogEventMetric(EventScope.Drafting)] - Task SetIsValidAsync(string userId, string projectId, int book, int chapter, bool draftApplied); + Task SetIsValidAsync(string userId, string projectId, int book, int chapter, bool isValid); Task SetRoleProjectPermissionsAsync(string curUserId, string projectId, string role, string[] permissions); Task SetUserProjectPermissionsAsync(string curUserId, string projectId, string userId, string[] permissions); } diff --git a/src/SIL.XForge/DataAccess/BsonValueConverter.cs b/src/SIL.XForge/DataAccess/BsonValueConverter.cs index 6cd22dfdbf..0e4bea9aab 100644 --- a/src/SIL.XForge/DataAccess/BsonValueConverter.cs +++ b/src/SIL.XForge/DataAccess/BsonValueConverter.cs @@ -23,6 +23,7 @@ public override BsonValue Read(ref Utf8JsonReader reader, Type typeToConvert, Js { bsonArray.Add(Read(ref reader, typeof(BsonValue), options)); } + return bsonArray; } diff --git a/src/SIL.XForge/DataAccess/MemoryRepository.cs b/src/SIL.XForge/DataAccess/MemoryRepository.cs index d280543e78..5d3cd0e4e0 100644 --- a/src/SIL.XForge/DataAccess/MemoryRepository.cs +++ b/src/SIL.XForge/DataAccess/MemoryRepository.cs @@ -233,7 +233,7 @@ JsonSerializer serializer var bsonArray = new BsonArray(); while (reader.Read() && reader.TokenType != JsonToken.EndArray) { - bsonArray.Add(BsonValue.Create(reader.Value)); + bsonArray.Add(ReadJson(reader, typeof(BsonValue), null, false, serializer)); } return bsonArray; diff --git a/src/SIL.XForge/Services/EventMetricService.cs b/src/SIL.XForge/Services/EventMetricService.cs index c0fbc7f412..0cb5e4345f 100644 --- a/src/SIL.XForge/Services/EventMetricService.cs +++ b/src/SIL.XForge/Services/EventMetricService.cs @@ -91,8 +91,6 @@ private static BsonValue GetBsonValue(object? objectValue) => decimal value => new BsonDecimal128(value), DateTime value => new BsonDateTime(value), null => BsonNull.Value, - _ => BsonValue.Create( - JsonConvert.DeserializeObject>(JsonConvert.SerializeObject(objectValue)) - ), + _ => BsonDocument.Parse(JsonConvert.SerializeObject(objectValue)), }; } diff --git a/test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs b/test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs index 176756f804..a08bd26670 100644 --- a/test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs +++ b/test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs @@ -302,6 +302,41 @@ await env.Service.SaveEventMetricAsync( Assert.AreEqual(BsonNull.Value, eventMetric.Result); } + [Test] + public async Task SaveEventMetricAsync_ArraysOfObjects() + { + var env = new TestEnvironment(); + var objectValue = new TestClass { Records = [new TestRecord { ProjectId = Project01, UserId = User01 }] }; + Dictionary argumentsWithNames = new Dictionary { { "myObject", objectValue } }; + string expectedJson = JsonConvert.SerializeObject(objectValue); + Dictionary expectedPayload = new Dictionary + { + { "myObject", BsonDocument.Parse(expectedJson) }, + }; + + // SUT + await env.Service.SaveEventMetricAsync( + Project01, + User01, + EventType01, + EventScope01, + argumentsWithNames, + result: null, + exception: null + ); + + // Verify the saved event metric + EventMetric eventMetric = env.EventMetrics.Query().OrderByDescending(e => e.TimeStamp).First(); + Assert.AreEqual(EventScope01, eventMetric.Scope); + Assert.AreEqual(Project01, eventMetric.ProjectId); + Assert.AreEqual(User01, eventMetric.UserId); + Assert.AreEqual(EventType01, eventMetric.EventType); + Assert.IsTrue(env.PayloadEqualityComparer.Equals(expectedPayload, eventMetric.Payload)); + string actualJson = eventMetric.Payload["myObject"].ToJson().Replace(" ", string.Empty); + Assert.AreEqual(expectedJson, actualJson); + Assert.AreEqual(BsonNull.Value, eventMetric.Result); + } + private class TestEnvironment { public TestEnvironment() @@ -357,4 +392,15 @@ public TestEnvironment() public IRepository EventMetrics { get; } public IEventMetricService Service { get; } } + + public class TestClass + { + public HashSet Records = []; + } + + public record TestRecord + { + public string ProjectId { get; set; } = string.Empty; + public string UserId { get; set; } = string.Empty; + } }