Code review resolutions

Author: mikechu-optimizely

packages/optimizely-sdk/lib/plugins/odp/graphql_manager.ts

@@ -39,7 +39,7 @@ const EMPTY_JSON_RESPONSE = null;
  * Manager for communicating with the Optimizely Data Platform GraphQL endpoint
  */
 export interface IGraphQLManager {
-  fetchSegments(apiKey: string, apiHost: string, userKey: string, userValue: string, segmentsToCheck: string[]): Promise<string[]>;
+  fetchSegments(apiKey: string, apiHost: string, userKey: string, userValue: string, segmentsToCheck: string[]): Promise<string[] | null>;
 }
 
 /**
@@ -73,27 +73,34 @@ export class GraphqlManager implements IGraphQLManager {
    * @param userValue Associated value to query for the user key
    * @param segmentsToCheck Audience segments to check for experiment inclusion
    */
-  public async fetchSegments(apiKey: string, apiEndpoint: string, userKey: string, userValue: string, segmentsToCheck: string[]): Promise<string[]> {
-    if (segmentsToCheck?.length === 0) {
-      return EMPTY_SEGMENTS_COLLECTION;
+  public async fetchSegments(apiKey: string, apiEndpoint: string, userKey: string, userValue: string, segmentsToCheck: string[]): Promise<string[] | null> {
+    if (!apiEndpoint || !apiKey) {
+      this._logger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to fetch segments');
+      return null;
     }
+
     const parameters = new QuerySegmentsParameters({
       apiKey,
       apiEndpoint,
       userKey,
       userValue,
       segmentsToCheck,
     });
+
     const segmentsResponse = await this._odpClient.querySegments(parameters);
     if (!segmentsResponse) {
       this._logger.log(LogLevel.ERROR, 'Audience segments fetch failed (network error)');
+      return null;
+    }
+
+    if (segmentsToCheck?.length === 0) {
       return EMPTY_SEGMENTS_COLLECTION;
     }
 
     const parsedSegments = this.parseSegmentsResponseJson(segmentsResponse);
     if (!parsedSegments) {
       this._logger.log(LogLevel.ERROR, 'Audience segments fetch failed (decode error)');
-      return EMPTY_SEGMENTS_COLLECTION;
+      return null;
     }
 
     if (parsedSegments.errors?.length > 0) {

packages/optimizely-sdk/lib/plugins/odp/odp_client.ts

@@ -69,10 +69,6 @@ export class OdpClient implements IOdpClient {
   public async querySegments(parameters: QuerySegmentsParameters): Promise<string | null> {
     const { apiEndpoint, apiKey, httpVerb, userKey, userValue, segmentsToCheck } = parameters;
 
-    if (segmentsToCheck?.length === 0) {
-      return '';
-    }
-
     if (!apiEndpoint || !apiKey) {
       this._logger.log(LogLevel.ERROR, 'No ApiHost or ApiKey set before querying segments');
       return null;
@@ -83,6 +79,10 @@ export class OdpClient implements IOdpClient {
       return null;
     }
 
+    if (segmentsToCheck?.length === 0) {
+      return '';
+    }
+
     const method = httpVerb;
     const url = apiEndpoint;
     const headers = {
@@ -118,17 +118,17 @@ export class OdpClient implements IOdpClient {
   public async sendEvents(parameters: SendEventsParameters): Promise<number | null> {
     const { apiEndpoint, apiKey, httpVerb, events } = parameters;
 
-    if (events?.length === 0) {
+    if (!apiEndpoint || !apiKey) {
+      this._logger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to send ODP events');
       return null;
     }
 
-    if (!apiEndpoint || !apiKey) {
-      this._logger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to send ODP events');
+    if (events?.length === 0) {
       return null;
     }
 
     const method = httpVerb;
-    const url = apiEndpoint;
+    const url = apiEndpoint ?? '';
     const headers = {
       'Content-Type': 'application/json',
       'x-api-key': apiKey,

packages/optimizely-sdk/tests/graphQLManager.spec.ts

@@ -51,6 +51,7 @@ describe('GraphQLManager', () => {
     resetCalls(mockOdpClient);
   });
 
+
   it('should parse a successful response', () => {
     const validJsonResponse = `{
       "data": {
@@ -187,7 +188,7 @@ describe('GraphQLManager', () => {
 
     const segments = await manager.fetchSegments(VALID_ODP_PUBLIC_KEY, ODP_GRAPHQL_URL, FS_USER_ID, VALID_FS_USER_ID, SEGMENTS_TO_CHECK);
 
-    expect(segments).toHaveLength(0);
+    expect(segments).toBeNull();
     verify(mockErrorHandler.handleError(anything())).never();
     verify(mockLogger.log(LogLevel.ERROR, 'Audience segments fetch failed (decode error)')).once();
   });
@@ -202,7 +203,7 @@ describe('GraphQLManager', () => {
 
     const segments = await manager.fetchSegments(VALID_ODP_PUBLIC_KEY, ODP_GRAPHQL_URL, FS_USER_ID, VALID_FS_USER_ID, SEGMENTS_TO_CHECK);
 
-    expect(segments).toHaveLength(0);
+    expect(segments).toBeNull();
     verify(mockErrorHandler.handleError(anything())).never();
     verify(mockLogger.log(anything(), anyString())).once();
   });
@@ -214,7 +215,7 @@ describe('GraphQLManager', () => {
 
     const segments = await manager.fetchSegments(VALID_ODP_PUBLIC_KEY, ODP_GRAPHQL_URL, FS_USER_ID, VALID_FS_USER_ID, SEGMENTS_TO_CHECK);
 
-    expect(segments).toHaveLength(0);
+    expect(segments).toBeNull();
     verify(mockErrorHandler.handleError(anything())).never();
     verify(mockLogger.log(LogLevel.ERROR, 'Audience segments fetch failed (decode error)')).once();
   });
@@ -225,7 +226,7 @@ describe('GraphQLManager', () => {
 
     const segments = await manager.fetchSegments(VALID_ODP_PUBLIC_KEY, ODP_GRAPHQL_URL, FS_USER_ID, VALID_FS_USER_ID, SEGMENTS_TO_CHECK);
 
-    expect(segments).toHaveLength(0);
+    expect(segments).toBeNull();
     verify(mockLogger.log(LogLevel.ERROR, 'Audience segments fetch failed (network error)')).once();
   });
 });

packages/optimizely-sdk/tests/odpSendEvents.spec.ts

@@ -80,11 +80,9 @@ describe('OdpClient Send Events', () => {
 
     expect(statusReturned).toBeNull();
     verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to send ODP events')).once();
   });
 
   it('should handle missing API Key', async () => {
-
     const missingApiKey = new SendEventsParameters({
       apiKey: '',
       apiEndpoint: 'https://some.example.com/endpoint',
@@ -96,7 +94,6 @@ describe('OdpClient Send Events', () => {
 
     expect(statusReturned).toBeNull();
     verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to send ODP events')).once();
   });
 
   it('Browser: should send events successfully', async () => {