This repository has been archived by the owner on Nov 5, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1
Update dv360 query to deal with advertiser-level query #62
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4b50b34
Update dv360 query to deal with advertiser-level query
hanj7221 60cda34
Fixing issues in comments and updating test
hanj7221 ad617bf
Changing into radio button and other minor changes
hanj7221 bb94a7d
Removing sanity checks in public api parser
hanj7221 e6b6056
Merge branch 'master' into advertiser-level-query
hanj7221 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,9 @@ import 'util.dart'; | |
placeholder="Insertion Order ID: 8127549" debugId="io-id-input"> | ||
<br> | ||
|
||
<input type="checkbox" [(ngModel)]="isAdvertiserQuery" name="advertiser-query"> | ||
<input type="radio" [(ngModel)]="advertiserQuery" name="advertiser-query"> | ||
<label for="advertiser-query">By advertiser</label><br> | ||
<input type="checkbox" [(ngModel)]="isInsertionOrderQuery" name="advertiser-query"> | ||
<input type="radio" [(ngModel)]="insertionOrderQuery" name="advertiser-query"> | ||
<label for="advertiser-query">By insertion order</label><br> | ||
|
||
<input type="checkbox" [(ngModel)]="highlightUnderpacing" | ||
|
@@ -31,7 +31,11 @@ import 'util.dart'; | |
{{buttonName}} | ||
</button> | ||
''', | ||
providers: [ClassProvider(QueryService), ClassProvider(ExcelDart)], | ||
providers: [ | ||
ClassProvider(QueryService), | ||
ClassProvider(ExcelDart), | ||
FORM_PROVIDERS, | ||
], | ||
directives: [coreDirectives, formDirectives], | ||
) | ||
class QueryComponent { | ||
|
@@ -45,17 +49,18 @@ class QueryComponent { | |
String insertionOrderId; | ||
bool highlightUnderpacing = false; | ||
|
||
QueryComponent(this._queryService, this._excel); | ||
|
||
bool get isAdvertiserQuery => _queryType == QueryType.byAdvertiser; | ||
set isAdvertiserQuery(bool checked) => | ||
checked ? _queryType = QueryType.byAdvertiser : null; | ||
// Radio button states with byAdvertiser selected as default. | ||
RadioButtonState advertiserQuery = RadioButtonState(true, 'byAdvertiser'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the radio button can only take string values :( Saw other people complain about this too. |
||
RadioButtonState insertionOrderQuery = RadioButtonState(false, 'byIO'); | ||
|
||
bool get isInsertionOrderQuery => _queryType == QueryType.byInsertionOrder; | ||
set isInsertionOrderQuery(bool checked) => | ||
checked ? _queryType = QueryType.byInsertionOrder : null; | ||
QueryComponent(this._queryService, this._excel); | ||
|
||
void onClick() async { | ||
// Determines the query type from radio buttons. | ||
_queryType = advertiserQuery.checked | ||
? QueryType.byAdvertiser | ||
: QueryType.byInsertionOrder; | ||
|
||
// Uses DV360 public APIs to fetch entity data. | ||
var insertionOrders = await _queryAndParseInsertionOrderEntityData(); | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanted to reply to this as part of a review to draw your attention to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.