TDL-13967: Add ProfitAndLoss report stream #37

savan-chovatiya · 2021-07-26T06:05:35Z

Description of change

TDL-13967: Add ProfitAndLoss report stream

Finalized approach:

Fetch records for a minimum of 30 days if the bookmark is used from the state in sync.
- If the bookmark is more than 30 days away then fetch all records up to the current day.
- If the bookmark is less than 30 days away then fetch records for the last 30 days.
Fetch records from start date to current date if the start date is used in sync.
Bookmark is required so consider it as an INCREMENTAL stream.
Consider ‘ReportDate’ as a primary key and replication key.
Only consider Accrual accounting method for the report as of now.

Manual QA steps

Verified that record for every day is emitted from the start date to current date in start date scenario.
Verified that a minimum 30-day record is emitted if a bookmark from the state is used.

Risks

Rollback steps

revert this branch

tap_quickbooks/streams.py

tests/unittests/test_reports_parser.py

luandy64 · 2021-08-26T15:59:44Z

luandy64 force-pushed the TDL-13967-add-profit-loss-report-stream branch from 8f786ff to b7b19cd

Ignore me. I was trying to reason about the report parser and wrote a test, but did not see that one was written in test_report_parser.py

luandy64 · 2021-08-26T16:10:58Z

tap_quickbooks/streams.py

+            params["end_date"] = end_tm_str
+
+            resp = self.client.get(self.endpoint, params=params)
+            self.parse_report_metadata(resp)


I think having parse_report_metadata() be recursive is excessive. The response object here is just three keys, Header (which we ignore), Rows, and Columns.

Parsing resp['Columns'] seems like a loop over resp['Columns']['Column'] and filtering the 'Metadata'.

Parsing resp['Rows'] is the only recursive part, so we can repurpose most of the logic in parse_report_metadata() into parse_rows() or something

Split down parse_report_metadata() into two functions parse_report_columns() and parse_report_rows().

parse_report_columns() will parse resp['Columns'] with a loop over resp['Columns']['Column'] and filters Metadata.

parse_report_rows() is the recursive one which will parse resp['Rows'].

dbshah1212 · 2021-09-22T10:36:53Z

tap_quickbooks/streams.py

+                'data': []
+            }
+
+            start_tm_str = strftime(start_dttm)[0:10]


Use str(start_dttm.date()) to get date

Updated date retrieval as per suggestion.

KrisPersonal · 2021-09-30T18:55:36Z

tap_quickbooks/streams.py

+        columns = pileOfColumns.get('Column', [])
+        for column in columns:
+            metadatas = column.get('MetaData', [])
+            for md in metadatas:


I prefer instead of using md have more descriptive variable name like metadata

Updated md with proper variable name metadata

KrisPersonal · 2021-09-30T18:56:28Z

tap_quickbooks/streams.py

+        '''
+
+        if isinstance(pileOfRows, list):
+            for x in pileOfRows:


Stop using variable names like x. Please provide more descriptive

Updated x with descriptive name row

KrisPersonal · 2021-09-30T18:57:14Z

tap_quickbooks/streams.py

+                self.parse_report_rows(pileOfRows['Summary'])
+
+            if 'ColData' in pileOfRows.keys():
+                d = dict()


Stop using variable names like d. Provide more descriptive variable name. Bad coding practice

I will keep in mind using descriptive variable names and updated above d with a proper name.

KrisPersonal · 2021-09-30T18:58:01Z

tap_quickbooks/streams.py

+                for x in pileOfRows['ColData'][1:]:
+                    vals.append(x['value'])
+                d['values'] = vals
+                self.parsed_metadata['data'].append(d)


No x, d variable names. Provide descriptive names

Updated x,d types of variables with proper descriptive names.

KrisPersonal · 2021-09-30T19:00:57Z

tap_quickbooks/streams.py

+            reports = self.day_wise_reports()
+            if reports:
+                for report in reports:
+                    yield report


What is the difference between yield report and yield from report? I've seen in many places using yield from report

yield from reports and yield inside for loops, both return generators with every element of reports.

The python generator will flush out after iteration over it and here, reports is a generator, and tap use last report from reports for storing bookmark.

If we use yield from reports then no way to retrieve the last report after it so we used yield with for loop here to utilize the loop's local variable report to retrieve the last report.

tap-quickbooks/tap_quickbooks/streams.py

Line 260 in b6e8e4a

self.state = singer.write_bookmark(self.state, self.stream_name, 'LastUpdatedTime', strptime_to_utc(report.get('ReportDate')).isoformat())

KrisPersonal · 2021-09-30T19:02:12Z

tap_quickbooks/streams.py

@@ -193,6 +198,151 @@ class Vendors(Stream):
    table_name = 'Vendor'
    additional_where = "Active IN (true, false)"

+class ReportStream(Stream):


Please add more comments to the code to explain at each section what is being done

Added more comments to the code for better code explanation.

KrisPersonal · 2021-09-30T19:10:56Z

tap_quickbooks/streams.py

+            if 'Rows' in pileOfRows.keys():
+                self.parse_report_rows(pileOfRows['Rows'])
+
+            if 'Row' in pileOfRows.keys():
+                self.parse_report_rows(pileOfRows['Row'])


What is the difference between Rows and Row here?

Rows is top-level information about profit and loss reports while Row is information about entries of reports. Documentation of the profit and loss report's metadata: documentation.

kspeer825

Left a suggestion.

kspeer825 · 2021-11-15T14:08:35Z

tests/base.py

@@ -230,3 +244,14 @@ def minimum_record_count_by_stream(self):
        record_counts["vendors"] = 26

        return record_counts
+
+    def dt_to_ts(self, dtime):


Raising an exception after this for loop would prevent the test from getting back a None and failing in a confusing way in the case where the datetime format is unaccounted for.

savan-chovatiya added 2 commits July 23, 2021 19:12

TDL-13967: Added ProfitAndLoss report stream

efb1458

TDL-13967: Handled state updation if no records found

4f4af32

savan-chovatiya requested review from karanpanchal-crest and dbshah1212 July 29, 2021 04:35

dbshah1212 reviewed Aug 4, 2021

View reviewed changes

tap_quickbooks/streams.py Outdated Show resolved Hide resolved

dbshah1212 reviewed Aug 4, 2021

View reviewed changes

tests/unittests/test_reports_parser.py Show resolved Hide resolved

dbshah1212 approved these changes Aug 4, 2021

View reviewed changes

savan-chovatiya added 3 commits August 4, 2021 16:10

Added comments in unittests

f25d3c0

Updated sync function for report streams

2237b71

Fixed integration tests

9ea6c51

savan-chovatiya requested a review from dbshah1212 August 18, 2021 07:36

Added stream into readme

4ac9530

dbshah1212 requested review from kspeer825 and KAllan357 August 19, 2021 10:16

dbshah1212 approved these changes Aug 19, 2021

View reviewed changes

Merge branch 'master' into TDL-13967-add-profit-loss-report-stream

b7b19cd

luandy64 force-pushed the TDL-13967-add-profit-loss-report-stream branch from 8f786ff to b7b19cd Compare August 26, 2021 15:58

luandy64 reviewed Aug 26, 2021

View reviewed changes

Break down report parser for rows and columns

18365fe

savan-chovatiya requested a review from luandy64 August 27, 2021 12:48

savan-chovatiya added 2 commits August 31, 2021 11:34

Resolved merge conflicts

84f2857

Resolved circleci failure

fd07ad1

savan-chovatiya requested a review from cosimon September 9, 2021 04:44

dbshah1212 reviewed Sep 22, 2021

View reviewed changes

savan-chovatiya added 2 commits September 22, 2021 16:29

Updated date retrival from datetime object

bf7d920

Break down dev and test ddependancy in setup

2cf255d

savan-chovatiya requested review from zachharris1 and removed request for KAllan357 September 23, 2021 10:44

KrisPersonal reviewed Oct 5, 2021

View reviewed changes

savan-chovatiya added 2 commits October 5, 2021 12:07

TDL-13967: Resolved review comments

604c67d

TDL-13967: Added code comments

b6e8e4a

savan-chovatiya requested a review from KrisPersonal October 5, 2021 09:01

luandy64 approved these changes Oct 5, 2021

View reviewed changes

KrisPersonal approved these changes Oct 6, 2021

View reviewed changes

dbshah1212 mentioned this pull request Oct 6, 2021

Tdl 13964 improve quickbooks reliability #46

Merged

KrisPersonal mentioned this pull request Oct 6, 2021

Bump to version 1.1.0 #47

Merged

kspeer825 approved these changes Nov 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-13967: Add ProfitAndLoss report stream #37

TDL-13967: Add ProfitAndLoss report stream #37

savan-chovatiya commented Jul 26, 2021 •

edited

Loading

luandy64 commented Aug 26, 2021

luandy64 Aug 26, 2021

savan-chovatiya Aug 27, 2021

dbshah1212 Sep 22, 2021

savan-chovatiya Sep 22, 2021

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021 •

edited

Loading

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021 •

edited

Loading

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021 •

edited by dbshah1212

Loading

KrisPersonal Sep 30, 2021

savan-chovatiya Oct 5, 2021 •

edited by dbshah1212

Loading

kspeer825 left a comment

kspeer825 Nov 15, 2021

TDL-13967: Add ProfitAndLoss report stream #37

Are you sure you want to change the base?

TDL-13967: Add ProfitAndLoss report stream #37

Conversation

savan-chovatiya commented Jul 26, 2021 • edited Loading

Description of change

Manual QA steps

Risks

Rollback steps

luandy64 commented Aug 26, 2021

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

savan-chovatiya Oct 5, 2021 • edited Loading

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

savan-chovatiya Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savan-chovatiya Oct 5, 2021 • edited by dbshah1212 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savan-chovatiya Oct 5, 2021 • edited by dbshah1212 Loading

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savan-chovatiya commented Jul 26, 2021 •

edited

Loading

savan-chovatiya Oct 5, 2021 •

edited

Loading

savan-chovatiya Oct 5, 2021 •

edited

Loading

savan-chovatiya Oct 5, 2021 •

edited by dbshah1212

Loading

savan-chovatiya Oct 5, 2021 •

edited by dbshah1212

Loading