Update dv360 query to deal with advertiser-level query #62

hanj7221 · 2020-07-15T15:12:11Z

closes #10

hanj7221 · 2020-07-15T15:22:45Z

lib/src/query_component.dart

+
+    // TODO: update reporting query to deal with multiple IOs.
+    // Issue: https://github.com/googleinterns/dv360-excel-plugin/issues/61
+    final insertionOrder = insertionOrders[0];


This is still querying the spend data for a single IO. Reporting api will be updated in the next pr.

ruben-aghayan · 2020-07-15T16:55:22Z

lib/src/dv360_service_response_parser.dart

@@ -2,7 +2,7 @@ import 'dart:convert';

 import 'proto/insertion_order_query.pb.dart';

-class InsertionOrderParser {
+class Dv360ServiceResponseParser {


nit: public api parser maybe? service response parser just sounds weird

ruben-aghayan · 2020-07-15T16:57:39Z

lib/src/dv360_service_response_parser.dart

+      map = json.decode(jsonString);
+    } catch (e) {
+      // [jsonString] is not in valid json format, returns empty list.
+      print('invalid json format');


rm debug code

so sorry, I think I applied the wrong git stash...

ruben-aghayan · 2020-07-15T16:58:22Z

lib/src/dv360_service_response_parser.dart

+    try {
+      map = json.decode(jsonString);
+    } catch (e) {
+      // [jsonString] is not in valid json format, returns empty string.


has this been happening? do we want to swallow the error

In real cases, no. And in component testing with mocks, null will be passed into this function.
I guess sanity check and return empty string will be better then.

let's keep throwing. an error may be introduced and we wont notice especially because we dont have a logging framework here. later we can have some top level error handling to fail more gracefully.

ruben-aghayan · 2020-07-15T16:59:17Z

lib/src/query_component.dart

+  set isAdvertiserQuery(bool checked) =>
+      checked ? _queryType = QueryType.byAdvertiser : null;
+
+  bool get isInsertionOrderQuery => _queryType == QueryType.byInsertionOrder;


why translate your enum to a set of bools?

I haven't give much thought to how the UI will look like with different types of queries. But for developing purpose, I think a checkbox is most convenient for me now?

It looks something like this:

And the bool is to have it work with the checkbox.

I think there should be some way to get radio buttons to work with an enum

ruben-aghayan · 2020-07-15T17:01:15Z

lib/src/query_component.dart

+    return insertionOrders
+        .where((io) =>
+            io.budget.activeBudgetSegment !=
+            InsertionOrder_Budget_BudgetSegment())


this doesnt need to check dates?

If the IO is not in-flight, it will have an empty InsertionOrder_Budget_BudgetSegment() for the activeBudgetSegment field.

Date checking has already been done during parsing.

ruben-aghayan · 2020-07-15T17:04:06Z

lib/src/query_component.dart

+    final insertionOrderList = <InsertionOrder>[];
+    var jsonResponse = '';
+
+    do {


nice do while loop

ruben-aghayan · 2020-07-15T17:04:34Z

lib/src/query_component.dart

+          Dv360ServiceResponseParser.parseNextPageToken(jsonResponse);
+
+      // Executes dv360 query, parses the response and adds results to the list.
+      var response = await _queryService.execDV3Query(


ruben-aghayan · 2020-07-15T17:08:11Z

lib/src/query_service.dart

+                  '$advertiserId/insertionOrders?$filter&$pageToken',
+              method: 'GET');
+        }
+        break;


do you need this break statement? is the return sufficient

ruben-aghayan · 2020-07-15T17:08:42Z

test/insertion_order_parser_test.dart

-    });
+    group('parses insertion orders from:', () {
+//      test('null', () {
+//        final actual = Dv360ServiceResponseParser.parseInsertionOrders(null);


ugh so sorry I think I applied the wrong git stash...

ruben-aghayan · 2020-07-15T17:09:12Z

test/insertion_order_parser_test.dart

@@ -1,10 +1,10 @@
 import 'package:angular_test/angular_test.dart';


update test file name to reflect class/file under test

ruben-aghayan · 2020-07-15T18:15:18Z

lib/src/public_api_parser.dart

+  ///
+  /// Returns an empty list if [jsonString] is null or empty.
+  static List<InsertionOrder> parseInsertionOrders(String jsonString) {
+    if (jsonString == null || jsonString.isEmpty) return [];


kind of concerned about the possibility of null or empty values popping up in any method. If a response was invalid i think we should fail and throw rather than propagate a null or empty string. it'll also make your life easier i think being confident in your inputs.

I think in real cases that jsonString can never be null or empty, since even if a request fails, it still returns a json object with the error message. I'm thinking that we should add parsing to the error json and display the error to users to let them know that they have entered the wrong id, etc. I guess we should do this for all queries.

i will create an issue and address this at end when all the queries are set.

And the sanity check is actually just for component testing, since mock class will pass in null to this function.

You can set the mock class to return a default value in setUp. i think it's better to facilitate testing needs in the test code rather than your business code

wanted to reply to this as part of a review to draw your attention to it

Ohh right. I will just get rid of the sanity checks then. The TODO for adding parsing support to error is at the top of the file.

ruben-aghayan · 2020-07-15T18:16:05Z

lib/src/public_api_parser.dart

    Map<String, dynamic> map = json.decode(jsonString);
-    return _createInsertionOrder(map);
+    return map.containsKey('insertionOrders')
+        ? List.from(map['insertionOrders'])


meta comment this is about as big as a ternary should get from a personal readability preference.

ruben-aghayan · 2020-07-15T18:17:27Z

lib/src/query_component.dart

@@ -16,9 +17,16 @@ import 'reporting_query_parser.dart';
    <input [(ngModel)]="insertionOrderId" 
           placeholder="Insertion Order ID: 8127549" debugId="io-id-input">
    <br>
+
+    <input type="checkbox" [(ngModel)]="isAdvertiserQuery" name="advertiser-query">


if the query types are mutually exclusive the affordances should reflect that - typically a radio button

ruben-aghayan · 2020-07-15T18:18:44Z

lib/src/query_service.dart

+      String nextPageToken, String advertiserId, String insertionOrderId) {
+    switch (queryType) {
+      case QueryType.byAdvertiser:
+        {


i think the inner brackets arent necessary?

looks like these are still here

Ohhh..I thought i can only get rid of brackets if the statement is one line.

hanj7221 · 2020-07-15T21:34:09Z

lib/src/query_component.dart

-  set isAdvertiserQuery(bool checked) =>
-      checked ? _queryType = QueryType.byAdvertiser : null;
+  // Radio button states with byAdvertiser selected as default.
+  RadioButtonState advertiserQuery = RadioButtonState(true, 'byAdvertiser');


Seems like the radio button can only take string values :(

Saw other people complain about this too.

ruben-aghayan · 2020-07-16T17:39:42Z

lib/src/query_service.dart

+      String nextPageToken, String advertiserId, String insertionOrderId) {
+    switch (queryType) {
+      case QueryType.byAdvertiser:
+        {


looks like these are still here

ruben-aghayan · 2020-07-16T17:41:35Z

lib/src/public_api_parser.dart

+  ///
+  /// Returns an empty list if [jsonString] is null or empty.
+  static List<InsertionOrder> parseInsertionOrders(String jsonString) {
+    if (jsonString == null || jsonString.isEmpty) return [];


wanted to reply to this as part of a review to draw your attention to it

Update dv360 query to deal with advertiser-level query

4b50b34

hanj7221 added this to the P0: query all IOs under a specific advertiser milestone Jul 15, 2020

hanj7221 requested a review from ruben-aghayan July 15, 2020 15:12

hanj7221 self-assigned this Jul 15, 2020

hanj7221 commented Jul 15, 2020

View reviewed changes

ruben-aghayan suggested changes Jul 15, 2020

View reviewed changes

Fixing issues in comments and updating test

60cda34

hanj7221 requested a review from ruben-aghayan July 15, 2020 18:06

ruben-aghayan suggested changes Jul 15, 2020

View reviewed changes

Changing into radio button and other minor changes

ad617bf

hanj7221 commented Jul 15, 2020

View reviewed changes

hanj7221 requested a review from ruben-aghayan July 15, 2020 21:34

ruben-aghayan suggested changes Jul 16, 2020

View reviewed changes

ruben-aghayan marked this pull request as draft July 16, 2020 17:40

ruben-aghayan suggested changes Jul 16, 2020

View reviewed changes

Removing sanity checks in public api parser

bb94a7d

hanj7221 marked this pull request as ready for review July 16, 2020 18:11

hanj7221 requested a review from ruben-aghayan July 16, 2020 18:11

ruben-aghayan approved these changes Jul 16, 2020

View reviewed changes

Merge branch 'master' into advertiser-level-query

e6b6056

hanj7221 merged commit 77f1cb1 into master Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dv360 query to deal with advertiser-level query #62

Update dv360 query to deal with advertiser-level query #62

hanj7221 commented Jul 15, 2020

hanj7221 Jul 15, 2020 •

edited

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020 •

edited

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020 •

edited

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

hanj7221 Jul 15, 2020

ruben-aghayan Jul 16, 2020 •

edited

ruben-aghayan Jul 16, 2020

hanj7221 Jul 16, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 15, 2020

ruben-aghayan Jul 16, 2020

hanj7221 Jul 16, 2020

hanj7221 Jul 15, 2020

ruben-aghayan Jul 16, 2020

ruben-aghayan Jul 16, 2020

		@@ -1,10 +1,10 @@
		import 'package:angular_test/angular_test.dart';

Update dv360 query to deal with advertiser-level query #62

Update dv360 query to deal with advertiser-level query #62

Conversation

hanj7221 commented Jul 15, 2020

hanj7221 Jul 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanj7221 Jul 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanj7221 Jul 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruben-aghayan Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanj7221 Jul 15, 2020 •

edited

hanj7221 Jul 15, 2020 •

edited

hanj7221 Jul 15, 2020 •

edited

ruben-aghayan Jul 16, 2020 •

edited