Skip to content
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

Work for 1174 #1177

Merged
merged 6 commits into from Aug 13, 2019

Conversation

@sbrossie
Copy link
Member

commented Aug 1, 2019

No description provided.

sbrossie added some commits Aug 1, 2019

invoice: Reimplement Invoice#populateChildren for one invoice by fetc…
…hing only the relevant rows -- and not fetch everything by account recordId. See #1174

@sbrossie sbrossie requested a review from pierre Aug 1, 2019

@pierre
Copy link
Member

left a comment

One Preconditions to fix.

Also, do we have tests to verify the two versions of populateChildren do the same thing (same name, but now very different implementation)? Just concerned about maintainability in the future (if we fix one, the other needs to be fixed as well).

return;
}

Preconditions.checkState(mappings.size() == 1, String.format("Expected only one parent mapping for invoice", invoice.getId()));

This comment has been minimized.

Copy link
@pierre

pierre Aug 2, 2019

Member

⚠️ Missing format parameter in the String.format (also, Preconditions.checkState can call String.format for you - check the varargs signature one).

This comment has been minimized.

Copy link
@sbrossie

sbrossie Aug 2, 2019

Author Member

Ack will fix.

@@ -227,7 +228,13 @@ public InvoiceItemModelDao createAdjustmentItem(final EntitySqlDaoWrapperFactory
}

public void populateChildren(final InvoiceModelDao invoice, final List<Tag> invoicesTags, final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFactory, final InternalTenantContext context) {

This comment has been minimized.

Copy link
@pierre

pierre Aug 2, 2019

Member

Do we have HA tests that call this implicitly? Since we have to duplicate the logic, and the HA one is a bit subtle, would be good to make sure we didn't introduce any regression.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Aug 2, 2019

Author Member

This code is pulled anytime there is an invoice generation or a payment associated with an invoice -- just to name those 2 use cases -- so will definitely be part of our test suite for HA. There could be bugs, that we don't see and that are not covered by our tests, but the same could be said of our existing logic.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Aug 5, 2019

Author Member

@pierre Update the PR with additional tests. See 5a10a5f

@sbrossie

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Just concerned about maintainability in the future (if we fix one, the other needs to be fixed as well).

yes i agree, the perf optimization introduces two paths, which will need to be maintained when and if we make code changes in this part of the code; i am also not too thrilled about that :-(

sbrossie added some commits Aug 2, 2019

invoice. Add regression tests to verify the various implementation of…
… Invoice#populateChildren leads to the same result. See #1174
@pierre

This comment has been minimized.

Copy link
Member

commented on 5a10a5f Aug 6, 2019

👍

@sbrossie sbrossie merged commit a6ad880 into work-for-release-0.21.x Aug 13, 2019

4 of 5 checks passed

ci/circleci: integration-tests Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-h2 Your tests passed on CircleCI!
Details
ci/circleci: test-mysql Your tests passed on CircleCI!
Details
ci/circleci: test-postgresql Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.