-
Notifications
You must be signed in to change notification settings - Fork 126
Add support for include_institution_data flag #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ac2757d
609a4b8
844bba6
6477ca9
40daae9
639139d
65f9f7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,8 +59,17 @@ public void testAssetReportGetSuccess() throws Exception { | |
| assertNotNull(respBody.getReport()); | ||
|
|
||
| // An Asset Report with Insights should include a name (when available). | ||
| assertNotNull(respBody.getReport().getItems().get(0).getAccounts().get(0).getTransactions() | ||
| .get(0).getName()); | ||
| assertTrue(containsTransactionWithName(respBody.getReport())); | ||
| } | ||
|
|
||
| private boolean containsTransactionWithName(AssetReportGetResponse.AssetReport assetReport) { | ||
| List<AssetReportGetResponse.Account> accounts = assetReport.getItems().get(0).getAccounts(); | ||
| for (AssetReportGetResponse.Account account : accounts) { | ||
| if (account.getTransactions().size() > 0) { | ||
| return account.getTransactions().get(0).getName() != null; | ||
|
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. Please confirm that a Also, I can't see this method being used anywhere, what is it meant for?
Contributor
Author
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. ah it was to prevent some flakiness but yes "" should return true, a report without insights will not have the name key |
||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,9 @@ | |
| import org.junit.Test; | ||
| import retrofit2.Response; | ||
|
|
||
| import static junit.framework.TestCase.assertNull; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
|
|
||
| public class InstitutionsSearchTest extends AbstractIntegrationTest { | ||
| @Test | ||
|
|
@@ -17,12 +19,38 @@ public void testSuccess() throws Exception { | |
| assertSuccessResponse(response); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSuccessWithIncludeInstitutionDataTrue() throws Exception { | ||
| Response<InstitutionsSearchResponse> response = | ||
| client().service().institutionsSearch(new InstitutionsSearchRequest("t").withIncludeInstitutionData(true)).execute(); | ||
|
|
||
| assertSuccessResponse(response); | ||
|
|
||
| InstitutionsSearchResponse institutionsSearchResponse = response.body(); | ||
| assertNotNull(institutionsSearchResponse.getInstitutions().get(0).getUrl()); | ||
| assertNotNull(institutionsSearchResponse.getInstitutions().get(0).getPrimaryColor()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSuccessWithIncludeInstitutionDataFalse() throws Exception { | ||
| Response<InstitutionsSearchResponse> response = | ||
| client().service().institutionsSearch(new InstitutionsSearchRequest("t").withIncludeInstitutionData(false)).execute(); | ||
|
|
||
| assertSuccessResponse(response); | ||
|
|
||
| InstitutionsSearchResponse institutionsSearchResponse = response.body(); | ||
|
|
||
| assertNull(institutionsSearchResponse.getInstitutions().get(0).getUrl()); | ||
| assertNull(institutionsSearchResponse.getInstitutions().get(0).getPrimaryColor()); | ||
| } | ||
|
|
||
|
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. These two tests look to me like a missed opportunity to reflect what difference setting |
||
| @Test | ||
| public void testNoResults() throws Exception { | ||
| Response<InstitutionsSearchResponse> response = | ||
| client().service().institutionsSearch(new InstitutionsSearchRequest("zebra")).execute(); | ||
|
|
||
| assertSuccessResponse(response); | ||
| assertEquals(0, response.body().getInstitutions().size()); | ||
|
|
||
| } | ||
| } | ||
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.
can we add a .withIncludeInstitutionData(boolean) instead of creating more constructors?
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.
yes will do
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.
Uh. I'm not a big fan of object mutation here. Can we either make it a static factory method (ie. static method that always returns a new object)?