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

Afform Entity Reference Submit #20305

Merged
merged 5 commits into from
May 21, 2021

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented May 14, 2021

Overview

Improve Afform Submit handling to handle for entity reference fields that reference other form elements

Before

Cannot use a value of Individual1 for a field such as source_contact_id for an Activity

After

Can use a value of Individual1 for field source_contact_id when creating an activity using Afform

Technical Details

This PR changes the Afform Submit Event dispatching so that it dispatches once per record / entity rather than once per afform submit

ping @colemanw @totten

@civibot
Copy link

civibot bot commented May 14, 2021

(Standard links)

@civibot civibot bot added the master label May 14, 2021
@seamuslee001 seamuslee001 force-pushed the afformEntityRefSubmit branch from 0ee3c91 to 77f4313 Compare May 14, 2021 10:19
@colemanw
Copy link
Member

This is shaping up nicely. Aside from a few nitpicks from the stylechecker, I just have a couple suggestions:

  1. Replace entity references before firing the submit event, so the listeners don't have to do all that work. Make a function for it and that function should check getfields to validate that the field is indeed an FK before attempting any replacements.
  2. Add an Afform.submit api4 test to show that a ref like activity_target = 'Contact1 works but that 'first_name' = "Contact2" doesn't get accidentally replaced just because it happens to be the name of an entity.
  3. Out of curiosity, is there any check being done to prevent self-references? E.g. a field in Contact1 references Contact1.id?

@seamuslee001
Copy link
Contributor Author

@colemanw I have done 1. but haven't looked into 2 & 3 as yet but doing 1 prompted some fixes so might be good to review it

@seamuslee001 seamuslee001 force-pushed the afformEntityRefSubmit branch from 7f094b5 to 9d4f6fb Compare May 19, 2021 09:26
@seamuslee001
Copy link
Contributor Author

@colemanw I believe Item 2 is done but not sure if item 3 is needed at this point

@colemanw
Copy link
Member

Thanks @seamuslee001 this looks good. I did some cleanup on related areas of code and realized that there's still more as I started to uncover some shortcomings in the preprocess/submit apis but if tests are passing now then this is in a good place to be merged.

@colemanw colemanw force-pushed the afformEntityRefSubmit branch 3 times, most recently from 44e86ce to 5dedaa3 Compare May 21, 2021 14:35
…coverage

- Simplifies getEntityWeights function using topological sorting library
- Consolodates postprocess event listners to just processGenericEntity
- Adds missing getters/setters to api classes
- Improves the AfformSubmitEvent interface to make it easeier to use
- Adds to tests
@colemanw colemanw force-pushed the afformEntityRefSubmit branch from 5dedaa3 to 5c89828 Compare May 21, 2021 15:37
@colemanw colemanw merged commit ddca0de into civicrm:master May 21, 2021
@colemanw
Copy link
Member

@seamuslee001 here's a funny story: I added a new unit test with a more complex scenario for Civi\Afform\Utils::getEntityWeights(), and was struggling to get it to pass. I had a hard time grokking that function to debug it and started to wonder if there was some code snippet on StackExchange I could steal. A quick web search landed me on https://github.com/marcj/topsort.php where I thought, "this looks nice but I don't want to introduce yet another dependency." Just when I was about to look elsewhere I did a double-take that one of the contributors was @totten! And lo and behold, it's already a core dependency (he used it to sort Extensions by order of dependencies).
5 minutes later I had it dropped into the getEntityWeights function and the test passed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants