wilcoxmc commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the issue

In the x_general_ledger.sql file, the transaction_index is added at the end, after the big union query. But when I try to connect to line records in the sub-ledgers, the index values don't always match up exactly. Perhaps you could added the index column to the sub-ledger queries where possibly, and then do an if_null or coalesce in the final query.

Relevant error log or model output

No response

Expected behavior

Expect to be able to connect GL lines entries to sub-ledger lines to be able to bring in additional columns like CLASS_ID.

dbt Project configurations

Name your project! Project names should contain only lowercase characters

and underscores. A good package name should reflect your organization's

name or the intended use of these models

name: 'dbt_QBO_GoForth' version: '1.0.0' config-version: 2

This setting configures which "profile" dbt uses for this project.

profile: 'dbt_QBO_GoForth'

These configurations specify where dbt should look for different types of files.

The `model-paths` config, for example, states that models in this project can be

found in the "models/" directory. You probably won't need to change these!

model-paths: ["models"] analysis-paths: ["analyses"] test-paths: ["tests"] seed-paths: ["seeds"] macro-paths: ["macros"] snapshot-paths: ["snapshots"]

target-path: "target" # directory which will store compiled SQL files clean-targets: # directories to be removed by dbt clean

"target"
"dbt_packages"

vars: quickbooks_database: GOFORTH quickbooks_schema: qbo_5tran # Where is your Quickbooks data?

# quickbooks_source:
#   using_address:        false         #disable if you don't have addresses in QuickBooks
#   using_bill:           false         #disable if you don't have bills or bill payments in QuickBooks
#   using_credit_memo:    false         #disable if you don't have credit memos in QuickBooks
#   using_department:     false         #disable if you don't have departments in QuickBooks
#   using_deposit:        false         #disable if you don't have deposits in QuickBooks
#   using_estimate:       false         #disable if you don't have estimates in QuickBooks
using_invoice:        false         #disable if you don't have invoices in QuickBooks
#   using_invoice_bundle: false         #disable if you don't have invoice bundles in QuickBooks
#   using_journal_entry:  false         #disable if you don't have journal entries in QuickBooks
#   using_payment:        false         #disable if you don't have payments in QuickBooks
#   using_refund_receipt: false         #disable if you don't have refund receipts in QuickBooks
#   using_transfer:       false         #disable if you don't have transfers in QuickBooks
#   using_vendor_credit:  false         #disable if you don't have vendor credits in QuickBooks
#   using_sales_receipt:  false         #disable if you don't have sales receipts in QuickBooks
#   using_purchase_order: true          #enable if you want to include purchase orders in your staging models

Package versions

packages:

package: fivetran/quickbooks version: [">=0.5.0", "<0.6.0"]

What database are you using dbt with?

snowflake

dbt Version

Core:

installed: 1.2.0-b1
latest: 1.1.1 - Ahead of latest version!

Plugins:

snowflake: 1.2.0b1 - Ahead of latest version!

Additional Context

No response

Are you willing to open a PR to help address this issue?

[ ] Yes.
[X] Yes, but I will need assistance and will schedule time during our office hours for guidance
[ ] No.

fivetran-joemarkiewicz commented 2 years ago

Thanks so much for opening this issue @wilcoxmc! We really appreciate you taking the time to raise this with our team.

@fivetran-avinash and I were looking through the package today and believe we found a good path forward! We checked the *_line source tables and found the proper index field is present in all of them. Therefore, we should be able to bring them through to the respective *_double_entry models and then finally include in the GL union. To get to this end result we are thinking of taking the following steps:

Ensure we are bringing in the index field within all staging models. I spot checked a few and saw that it was included, but want to confirm that is the case.
Add the index field into the respective *_double_entry models. I do want to be mindful though that the addition of this field doesn't break convention within of the general model. In particular, I would like to keep an eye out if adding the index field causes any issues within the debt/credit unions within these models.
Once we have the index included in the *_double_entry models, we can then include it in the quickbooks__general_ledger union. I did like your point though of making sure to do a coalesce in order to not omit the index entirely if it is not present. Therefore, we could attempt a possible solution like the following:
```
coalesce(gl_union.index, row_number() over(partition by gl_union.transaction_id order by gl_union.transaction_date)) as transaction_index,
```

I believe with these steps we can get the proper index included in the final GL model and allow for post package sub ledger joins! @fivetran-avinash is going to be looking further into implementing these changes. Feel free to keep an eye out for a PR in the future and please chime in if you would like discuss or test any of the changes as they are developed.

aufdenkamp commented 2 years ago

Would love updates on how this is going - I believe I'm having the exact same problem.

fivetran-avinash commented 2 years ago

Hello @aufdenkamp!

I have been able to open a PR #41 to bring in subindices from different models into the general ledger, and create a surrogate key as well. Would love to get your feedback on whether this fixes your issue before we deploy tomorrow!

Feel free to install the branch the PR is built from to test out the results:

packages:
  - git: https://github.com/fivetran/dbt_quickbooks.git
    revision: bug/quickbook_line_indexes
    warn-unpinned: false

et me know if the changes are what you expected, and we'll be able to deploy this live for you to use tomorrow 🙌 . Thanks!

aufdenkamp commented 2 years ago

Awesome, we will check it out tomorrow and let you know. I appreciate you reaching out!

On Tue, Aug 23, 2022 at 12:50 PM Avinash Kunnath @.***> wrote:

Hello @aufdenkamp https://github.com/aufdenkamp!

I have been able to open a PR #41 https://github.com/fivetran/dbt_quickbooks/pull/41 to bring in subindices from different models and create a surrogate key as well. Would love to get your feedback on whether this fixes your issue before we deploy tomorrow!

Feel free to install the branch the PR is built from to test out the results:

packages:

git: https://github.com/fivetran/dbt_quickbooks.git

revision: bug/quickbook_line_indexes

warn-unpinned: false

et me know if the changes are what you expected, and we'll be able to deploy this live for you to use tomorrow 🙌 . Thanks!

— Reply to this email directly, view it on GitHub https://github.com/fivetran/dbt_quickbooks/issues/40#issuecomment-1224460636, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXIZPOFFPQDL7RVHYXSHJIDV2UFORANCNFSM54MQX7YQ . You are receiving this because you were mentioned.Message ID: @.***>

aufdenkamp commented 2 years ago

After testing this branch, I don't believe this solved our issues. The only thing that appears to be off for us is the balance sheet - the month-end balances on some GL accounts (bank accounts, equity, accounts payable, accounts receivable) aren't matching up to what we show in Quickbooks. Some are significantly off, though total Assets DO balance with Liabilities + Equity as you'd expect. I'm not sure what's happening here, but it looks like it's a different issue.

fivetran-joemarkiewicz commented 2 years ago

Hey @aufdenkamp I agree that this seems to be a different issue 🤔

Would you mind opening a new Bug report around the discrepancies you are seeing and we can dig some more into that bug.

fivetran-joemarkiewicz commented 2 years ago

Hi @wilcoxmc thanks again for raising this to our team. We have since been able to include these updates in the latest release of the package.

@aufdenkamp thanks for raising the discrepancy with our team and opening Issue #43. We may continue conversations in that respective issue.

fivetran / dbt_quickbooks

[Bug] General Ledger transaction_index discrepancies with sub-ledger line indexes #40

Is there an existing issue for this?

Describe the issue

Relevant error log or model output

Expected behavior

dbt Project configurations

Name your project! Project names should contain only lowercase characters

and underscores. A good package name should reflect your organization's

name or the intended use of these models

This setting configures which "profile" dbt uses for this project.

These configurations specify where dbt should look for different types of files.

The `model-paths` config, for example, states that models in this project can be

found in the "models/" directory. You probably won't need to change these!

Package versions

What database are you using dbt with?

dbt Version

Additional Context

Are you willing to open a PR to help address this issue?

fivetran / dbt_quickbooks

[Bug] General Ledger transaction_index discrepancies with sub-ledger line indexes #40

Is there an existing issue for this?

Describe the issue

Relevant error log or model output

Expected behavior

dbt Project configurations

Name your project! Project names should contain only lowercase characters

and underscores. A good package name should reflect your organization's

name or the intended use of these models

This setting configures which "profile" dbt uses for this project.

These configurations specify where dbt should look for different types of files.

The model-paths config, for example, states that models in this project can be

found in the "models/" directory. You probably won't need to change these!

Package versions

What database are you using dbt with?

dbt Version

Additional Context

Are you willing to open a PR to help address this issue?

The `model-paths` config, for example, states that models in this project can be