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

Add ContributionProduct and EntityBatch APIv4 Entity #20505

Merged

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Jun 4, 2021

Overview

Add ContributionProduct and EntityBatch APIv4 Entity

Technical Details

I want to raise two points:

  1. Found that the ContributionProduct.product_id is missing foreign key attribute, will add a separate PR to add that.
  2. Not sure if we need add pseudoconstant property on EntityBatch.entity_table as I am not sure if it contain finite number of table list, right now I can think of civicrm_financial_trxn and civicrm_contact.

Comments

ping @colemanw @eileenmcnaughton @seamuslee001

@civibot
Copy link

civibot bot commented Jun 4, 2021

(Standard links)

@civibot civibot bot added the master label Jun 4, 2021
@eileenmcnaughton
Copy link
Contributor

@monishdeb I thought entity_batch was used for the batch entry screen - ie the screen altered by this pr #20394 & could hold contribution & membership

@eileenmcnaughton
Copy link
Contributor

api\v4\Entity\ConformanceTest::testConformance with data set "EntityBatch" ('EntityBatch')
Exception: At least one Batch is required in test

@monishdeb
Copy link
Member Author

monishdeb commented Jun 8, 2021

@monishdeb I thought entity_batch was used for the batch entry screen - ie the screen altered by this pr #20394 & could hold contribution & membership

@eileenmcnaughton to reconfirm I did a membership & contribution batch entry and clicked 'Save & Continue Later' but that doesn't populate the entity_batch table. But if you do 'Accounting Batch Entry' it does populate the entity_batch table with financial_trxn entries and not sure in which case civicrm_contact entries are populated in this table.

@eileenmcnaughton
Copy link
Contributor

@monishdeb I think that adding a pseudoconstant would ONLY affect apiv4 - so it should be Ok to only add the one you know to be true - the contact one seems odd

@monishdeb
Copy link
Member Author

monishdeb commented Jun 8, 2021

@monishdeb I think that adding a pseudoconstant would ONLY affect apiv4 - so it should be Ok to only add the one you know to be true - the contact one seems odd

Ok will add the pseudoconstant then. Regarding contact, its the description about the entity_table here tells about that civicrm_contact could be one of the entity_table, but not sure when the civicrm_entity_batch is used to hold contact entries.

On the otherhand, I will also submit a separate PR to make civicrm_contribution_product.product_id foreign key to civicrm_product.id (on delete null).

@eileenmcnaughton
Copy link
Contributor

@monishdeb do you it was maybe 'just a good idea' at some point? Perhaps we should merge directly after the rc is cut to be careful but I have doubts about including contact if it's not really possible to populate as that

@demeritcowboy you are our sleuth.... can you find any want for civicrm_entity_batch.entity_table to be anything other than civicrm_financial_trxn ?

@monishdeb monishdeb force-pushed the contribution_product_api4 branch from a4d960d to 6a657a5 Compare June 8, 2021 05:23
@monishdeb
Copy link
Member Author

jenkins test this please

@monishdeb
Copy link
Member Author

I have addressed point 1) under the Technical Details and submitted a PR for that #20553

@demeritcowboy
Copy link
Contributor

I've seen 3 things that use the word batch:

  • Accounting batches, which are effectively an "export" with tracking so you don't export the same twice.
    • batch and entity_batch, with entity_table civicrm_financial_trxn
  • Batch entry for contributions etc, which are effectively an "import".
    • Data here gets stored in civicrm_batch. The whole thing is stored in the data column. @monishdeb you're right that "save and continue later" doesn't use entity_batch, but if you go on to "validate and process" then it DOES also store a record in civicrm_entity_batch with entity_table civicrm_financial_trxn. One reason being that you can re-use these for "export" later as pre-made accounting batches.
  • Batch update via profile - the action on search results screens
    • This isn't really the same type of "persistent" batch.

I don't see anything obvious where entity_batch is used for non-financial-transaction, but there's nothing that says it couldn't be used by an extension for such. It seems there was an intent to allow it, but maybe nothing actually uses it for contacts etc:

It mentions Gift Aid - does someone know if that uses it for something other than financial?

@monishdeb monishdeb force-pushed the contribution_product_api4 branch from 6a657a5 to 7c08d7b Compare June 9, 2021 03:53
@eileenmcnaughton
Copy link
Contributor

@demeritcowboy @monishdeb I don't think resolving the pseudoconstant is a blocker on merging this.

However, I found that giftaid DOES use this table with civicrm_contribution.

I feel like we can somewhat rule out civicrm-contact based on discussion so far -but there DOES seem to be a need for extensions to edit what is supported.

At the moment I'm leaning towards using an option group - I'm gonna see if @totten @colemanw @seamuslee001 have thoughts

@eileenmcnaughton
Copy link
Contributor

I've transferred the above ^^ to https://lab.civicrm.org/dev/core/-/issues/2682 and am merging this - let's discuss over there

@eileenmcnaughton eileenmcnaughton merged commit 91c61b7 into civicrm:master Jul 8, 2021
@seamuslee001 seamuslee001 deleted the contribution_product_api4 branch July 9, 2021 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants