[Bug] Incorrect translation rate applied

[rwang-lyra]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the issue

NETSUITE2__BALANCE_SHEET is applying incorrect consolidated translation rate. Example is below however the issue applies to transactions in many other accounts.

Generate Rate Type on the COA is set to Historical. Based on this, the result of the query below should be $x USD. However, it returns $y USD.

The reason is because the Dec 2021 transaction is translating at the Jun 2023 Historical Rate, rather than the Dec 2021 Historical Rate.

Transaction in accounts set to translate at Historical Rates should refer back to the rate in the period they were recorded. In this case, that is Dec 2021.

Relevant error log or model output

No response

Expected behavior

expecting the rate shall be the one for the relative time for each records.

dbt Project configurations

packages:

• package: dbt-labs/dbt_utils version: 1.0.0
• package: brooklyn-data/dbt_artifacts version: 2.3.0
• package: dbt-labs/dbt_project_evaluator version: 0.5.0
• package: dbt-labs/dbt_external_tables version: 0.8.3
• package: fivetran/netsuite version: 0.8.0

Package versions

0.8.0

What database are you using dbt with?

snowflake

dbt Version

1.4.5

Additional Context

No response

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

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

[jmongerlyra]

The issue may be in the section below from netsuite2__balance_sheet.sql.

The account in question has an account_type of 'asset' but it has a general rate type of 'historical.' Transactions to an account with a generate rate type of 'historical' should translate at the original rate in all subsequent periods.

We should consider using converted_amount_using_transaction_accounting_period for converted_amount based on the general rate type set in the chart of accounts, rather than account_category or is_balancesheet.

    case
      when not accounts.is_balancesheet or lower(transactions_with_converted_amounts.account_category) = 'equity' then -converted_amount_using_transaction_accounting_period
      when not accounts.is_leftside then -converted_amount_using_reporting_month
      when accounts.is_leftside then converted_amount_using_reporting_month
      else 0
        end as converted_amount,

cc: @fivetran-reneeli

[fivetran-jamie]

hey there @rwang-lyra @jmongerlyra 👋

I think you're right that about that case statement...i'm curious if adjusting the logic in lines 68 and 149 to also look at the general_rate_type would do the trick, so like

    case
      when lower(accounts.general_rate_type) = 'historical' or not accounts.is_balancesheet or lower(transactions_with_converted_amounts.account_category) = 'equity' then -converted_amount_using_transaction_accounting_period
      when not accounts.is_leftside then -converted_amount_using_reporting_month
      when accounts.is_leftside then converted_amount_using_reporting_month
      else 0
        end as converted_amount,

i've made the adjustment in a working branch if you'd like to try it out! also please note that the package is up to v0.9.0 (looks like y'all are on v0.8.0) in case you notice anything that's different. here's the branch info

# packages.yml
packages:
  - git: https://github.com/fivetran/dbt_netsuite.git
    revision: scope/historical-rate-issue
    warn-unpinned: false

[jmongerlyra]

@fivetran-jamie I think we should remove lower(transactions_with_converted_amounts.account_category) = 'equity' from the CASE statement. NetSuite translation is driven by the general rate type rather than the account category.

It just happens to be that equity accounts should all be historical unless there are unusual requirements. Removing that statement will ensure equity accounts translate based on the reporting month in the event average or current rates are selected.

[fivetran-jamie]

Ah I see! So just using the general_rate_type is definitely a more complete solution. I've pushed the changes to the same branch if you wanna try it out @jmongerlyra

[jmongerlyra]

@fivetran-jamie Thanks! I noticed a sign issue in converted_amount since asset accounts have an is_leftside value of true. What about something like the following?

case
  when not accounts.is_leftside and (lower(accounts.general_rate_type) = 'historical' or not accounts.is_balancesheet) then -converted_amount_using_transaction_accounting_period
  when accounts.is_leftside and (lower(accounts.general_rate_type) = 'historical' or not accounts.is_balancesheet) then converted_amount_using_transaction_accounting_period
  when not accounts.is_leftside then -converted_amount_using_reporting_month
  when accounts.is_leftside then converted_amount_using_reporting_month
  else 0
    end as converted_amount

[fivetran-jamie]

I was wondering why we didn't split up the signs of converted_amount_using_transaction_accounting_period like how we treat converted_amount_using_reporting_month... So as it stands, the case statement does not appropriately capture historical asset account transactions?

[jmongerlyra]

That's correct. The converted_amount sign is wrong for certain account types without the revision above.

[fivetran-jamie]

thanks for confirming! just pushed your proposed change to the working branch if you'd like to test

[jmongerlyra]

@fivetran-jamie Looks good. You can close this out and merge. There are additional findings with the balance sheet table, but we will open separate issues. Thanks!

[fivetran-jamie]

Great! Someone on my team will get a PR open and polished for release soon.

This will probably get merged in and released early next sprint (starts this Thursday). So if you have time to open the other balance sheet issues this week, perhaps we can batch those together

[fivetran-avinash]

Hi @jmongerlyra , I reviewed your new case logic in this comment with our own data and how the converted amounts turn out with our master branch and the new scoping branch. While most of the values are unimpacted, there is one additional issue I found with the new logic for cases where

lower(account_category) = 'expense'  
and is_leftside = 'true' 
and is_balancesheet = 'false'
and lower(general_rate_type) = 'average'

The old logic would make this converted_amount negative because of the false on balance sheet condition. But because is_leftisde = 'false' now is the primary condition for converting to negative, this particular case will now become positive.

I was wondering if adding this tweak to the start of the case statement might fix this? It resolves our issues but am curious if you think it's too specific and there might be cases where we'd want to convert expenses to a positive value.

    case
      when lower(account_category) = 'expense' then -converted_amount_using_transaction_accounting_period

Let me know your thoughts! Your feedback will help us expedite shipping this logic live.

[jmongerlyra]

@fivetran-avinash Is that not handled by the code below? expense accounts should pass is_leftside and not accounts.is_balancesheet I might not be understanding how is_leftside is used in the model, but expense accounts are native debit balances.

when not accounts.is_leftside 
and (lower(accounts.general_rate_type) = 'historical' or not accounts.is_balancesheet) 
then -converted_amount_using_transaction_accounting_period

That code came from here.

[fivetran-avinash]

Hi @jmongerlyra! The logic is case when not acccounts.is_leftside, so expenses would not fall into that case statement, they'd fall into the next line, where it's case when accounts.is_leftside, where they'd return positive. So I added that additional line.

Are expenses passing as negative for you with your current balance sheet case logic? Are the expenses on your end not leftside?

Let me know if that all makes sense! Happy to answer any questions.

[jmongerlyra]

@fivetran-avinash Good catch! My eye skipped over the not and I misread your original note.

Is this patch solely being applied to the balance sheet table or others (like the income statement)? expense accounts don't impact the balance sheet except through retained earnings. expense totals should accumulate into retaining earnings as debit balances.

I am available next week to work through this live if you have availability. We have another bug in retained earnings, so this is likely relevant.

My concern with the code below is translation should be driven by general_rate_type rather than the account_category.

    case
      when lower(account_category) = 'expense' then -converted_amount_using_transaction_accounting_period

[fivetran-avinash]

Thanks for that @jmongerlyra. I do see most expenses going into retained earnings, but I also see some of it filter into 'Net Income' as well. Would that also not impact the balance sheet?

While it doesn't directly impact the balance sheet, I think we wouldn't want expense figures to be positive, ever. Because of the is_leftside = true logic though (which is inverted from other cases), I don't have a great answer for how to utilize general_rate_type here since I don't know how many cases there are for rate types for expenses that could switch that logic. So I could add logic like

case
      when lower(account_category) = 'expense' and  lower(accounts.general_rate_type) = 'historical' then -converted_amount_using_transaction_accounting_period
     when  lower(account_category) = 'expense' and  lower(accounts.general_rate_type) != 'historical'  then -converted_amount_using_transaction_accounting_period

But it's just repetitive at the moment, so I don't see the net add.

Are there any cases you could see where that expense logic wouldn't hold? I would love to get merged this current logic before the end of our sprint, but also don't want to rush toward a solution if there are unintended conseuqences.

Happy to set up time to chat next week through your other retained earnings issue. What times work best for you?

[jmongerlyra]

@fivetran-avinash I have emailed @fivetran-reneeli so we can setup a time to meet. I see now that the models present debits to expenses as negative amounts.

What about this (or something equivalent)?

    case
      when not accounts.is_balancesheet and lower(accounts.general_rate_type) in ('historical', 'average') then -converted_amount_using_transaction_accounting_period
      when not accounts.is_balancesheet then -converted_amount_using_reporting_month
      when accounts.is_balancesheet and not accounts.is_leftside and lower(accounts.general_rate_type) in ('historical', 'average') then -converted_amount_using_transaction_accounting_period
      when accounts.is_balancesheet and accounts.is_leftside and lower(accounts.general_rate_type) in ('historical', 'average') then converted_amount_using_transaction_accounting_period
      when accounts.is_balancesheet and not accounts.is_leftside then -converted_amount_using_reporting_month
      when accounts.is_balancesheet and accounts.is_leftside then converted_amount_using_reporting_month
      else 0
        end as converted_amount

This should fix the sign issue and doesn't rely on the account_category for translation. Feel free to modify. Unfortunately, I do not have access to rerun the models to test. I will need @rwang-lyra for that.

EDIT: @fivetran-avinash and @fivetran-jamie. I modified and tested the revised code above. This corrects the original translation issue as well as the net income and retained earnings variances we saw on today's call. I replaced rows 67-72 and 148-151. Results now match NetSuite.

CTA still needs further research, but we will address in another ticket. Let us know the results of your testing.

[jmongerlyra]

@fivetran-avinash This revision causes liability accounts and equity accounts to present as negative numbers. Is that the desired behavior? That is different than the presentation on the native NetSuite balance sheet report, and the current model.

[fivetran-avinash]

Hi @jmongerlyra, sorry did some additional digging when I realized my investigation was incomplete, so deleted my initial comment. Here's my follow-up:

In our internal data, I discovered a few additional edge cases that were converting values from negative to positive (or positive to negative) incorrectly. It's a good point you bring up on what is the best data to present as negative that accurately reflects the balance sheet report and thanks for bringing that this logic differs from what we see internally. I'm trying to figure out the best way to fix this up so that this logic works for all customers.

I called up the equity cases. First, here are the account_category=equity cases that change with your new logic.

`is_income_statement = false` && `is_balancesheet = true` && `is_leftside = false` && `general_rate_type in (`current`, `historical`)` && `balance_sheet_sort_helper = 16` (negative before, positive after)
`is_income_statement = true` && `is_balancesheet = false` && `is_leftside = false` && `general_rate_type in (`historical`, `average`) && `balance_sheet_sort_helper = 16` (negative before, positive after)
`is_income_statement = true` && `is_balancesheet = false` && `is_leftside = true` && `general_rate_type = `average` ) && `balance_sheet_sort_helper = 16 (positive before, negative after)

Here are the account_category=equity cases I found which were unchanged:

`is_income_statement = false` && `is_balancesheet = true` && `is_leftside = false` && `general_rate_type = `historical`` && `balance_sheet_sort_helper = 13` (positive before, positive after)
`is_income_statement = false` && `is_balancesheet = true` && `is_leftside = false` && `general_rate_type = `current`` && `balance_sheet_sort_helper = 16` (positive before, positive after)
`is_income_statement = false` && `is_balancesheet = true` && `is_leftside = true` && `general_rate_type = `historical` && `balance_sheet_sort_helper = 16` (negative before, negative after)

So I think we want to determine whether each of these cases should be positive or negative, and then I can sync back with my team. Based on your experience with Netsuite, what are your thoughts for all of the cases presented above?

[jmongerlyra]

@fivetran-avinash If I am reading this correctly, the only sign differences are within CTA (i.e. balance_sheet_sort_helper = 16).

CTA has other issues, so I think we should leave the converted_amount alone in that block, and only apply this revision to lines 67-72. I filed a separate issue on CTA and scheduled office hours.

Concerns with that approach?

[fivetran-avinash]

Hi @jmongerlyra this makes sense!

We've decided to hold off on merging this into our master branch until we resolve the CTA issue since we want to validate both fixes internally, but for now you can use this branch in your packages.yml to utilize your new logic!

# packages.yml
packages:
  - git: https://github.com/fivetran/dbt_netsuite.git
    revision: scope/historical-rate-issue
    warn-unpinned: false

Let us know how this works! And we look forward to resolving issue #79 as well and having this new logic fully deployed.

Thanks for your partnership here.

[jmongerlyra]

Sounds good @fivetran-avinash. The earliest office hours I could get on CTA was 9/19. If there is any earlier availability, I am open to meeting before then.

[fivetran-avinash]

Thanks @jmongerlyra. I will be out of the office on 9/19, but I think @fivetran-joemarkiewicz and @fivetran-jamie can attend and provide insight on how to help with CTA, that I can then use to work on this in a future sprint.

We did spot one issue in testing in our development data this new case logic. A fixed asset that had a historical rate type that had is_leftside=true and is_balancesheet=true was now using converted_amount_using_transaction_accounting_period instead of converted_amount_using_reporting_month. Do you believe that this is the proper way to translate that case?

[jmongerlyra]

Yes, if the GL account's general_rate_type is set to historical, it should translate using the consolidated exchange rate of the transaction's accounting period. That should match the NetSuite balance sheet. If it doesn't in Fivetran's test environment, let's discuss.

[fivetran-avinash]

Hi @jmongerlyra, we were able to confirm internally that your logic is correct! So we will be merging this new branch in shortly in our next release.

We are folding this fix into a larger netsuite release that will include a solve for the CTA issue you also filed. So for now would again recommend using the above configuration in your packages.yml to leverage our models until then and reflect the corrected translation rates.

Thanks for your patience and all your hard work in making this netsuite model better than ever.

[fivetran-joemarkiewicz]

Hi all! The updates highlighted in this issue have been merged and released in the latest release of the dbt_netsuite package. These updates should be live on the dbt hub at the top of the hour.

If you still see this issue persist after upgrading, please let us know. Closing this issue as the latest release includes the fix.