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

dev/core#5053 Afform - Add support for saving event location #30140

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented May 6, 2024

Overview

Fulfills feature request from https://lab.civicrm.org/dev/core/-/issues/5053

Before

Not possible to save email/phone/address when creating an event via Afform

After

Support added for those fields, in the form of a block.

Copy link

civibot bot commented May 6, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label May 6, 2024
Copy link

civibot bot commented May 6, 2024

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/5053

@mattwire
Copy link
Contributor

mattwire commented May 7, 2024

Test fail is probably related:

api\v4\Afform\AfformCustomFieldUsageTest::testMultiRecordCustomBlock
Failed asserting that null matches expected 'One'.

/home/homer/buildkit/build/build-3/web/sites/all/modules/civicrm/ext/afform/mock/tests/phpunit/api/v4/Afform/AfformCustomFieldUsageTest.php:102
/home/homer/buildkit/extern/phpunit9/phpunit9.phar:2307

@mattwire
Copy link
Contributor

mattwire commented May 7, 2024

Non-blocking comment but probably our only opportunity if we wanted to.. How hard would it be to rename the entity from LocBlock to LocationBlock?

@colemanw
Copy link
Member Author

colemanw commented May 7, 2024

Non-blocking comment but probably our only opportunity if we wanted to.. How hard would it be to rename the entity from LocBlock to LocationBlock?

The LocBlock entity is a horrible design and the name is the least of its problems. I wouldn't bother renaming it since what it really needs is to be ripped out.

'where' => [['id', '=', $mainEntityId]],
'select' => [$forwardFkField['name']],
])->first();
return [$joinIdField, '=', $mainEntityJoinValue[$forwardFkField['name']] ?? 0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw Should this return an array of current return value? I get the following error when trying to prefill the afform with event id in URL (happens only if location block is included).

TypeError: array_pad(): Argument #1 ($array) must be of type array, string given in array_pad() (line 303 of web/sites/all/modules/civicrm/Civi/Api4/Query/Api4SelectQuery.php).

@Guydn
Copy link

Guydn commented May 20, 2024

I have just started to test. On the back end there is a field to state whether it is a new location or not. This field does not exist in FB. Maybe I should add a custom field so that I display the existing location or not (cond rule).

I try to create an autocomplete on the addresses. It works well in SK (with address, postal code, town). In form builder, I can set the Search and the autocomplete display. But when i run the form, it only shows the ID and does not apply the criteria (to limit the number of addresses).

@colemanw
Copy link
Member Author

Thanks for testing @Guydn
I should clarify that this PR exposes the entities to Afform but does not attempt to reproduce all the complex features of the "Location" tab on the "Event" screen.
If you let me know what are the most important features for you I can try to work on them.

@Guydn
Copy link

Guydn commented May 20, 2024

Yes sure. In the membership organisation, around 50 people are able to create events (and modify). They do not have access to CiviCRM but to a web logged in environment on WP. They are able to choose a event template (that brings a lot of information) and have to define a location (either select one or create one), description, title, dates, quota.
I have a quick form which has been working for 2,5 years now so it is not urgent.
The location date structure is complex and honestly, for events, I do not have a clear understanding (we have many duplicates...)
Civicrm event location
CiviCRM event location 2
Hoping it clarifies.

@colemanw colemanw force-pushed the afformLocBlock branch 2 times, most recently from 7fdff5c to e5dbfd5 Compare August 10, 2024 17:40
@colemanw
Copy link
Member Author

@Guydn I've added support for autocomplete + autofill of location info. It was extremely challenging!

@Guydn
Copy link

Guydn commented Aug 10, 2024

@Guydn I've added support for autocomplete + autofill of location info. It was extremely challenging!

Thanks a lot @colemanw Yes i understand that it is a very very difficult development.

@allinappliadmin
Copy link
Contributor

allinappliadmin commented Aug 12, 2024

I can't merge patch on 5.76.1 as a file is missing

patching file Civi/Api4/Action/LocBlock/Create.php
patching file Civi/Api4/Action/LocBlock/LocBlockSaveTrait.php
patching file Civi/Api4/Action/LocBlock/Save.php
patching file Civi/Api4/Action/LocBlock/Update.php
patching file Civi/Api4/Generic/Traits/DAOActionTrait.php
patching file Civi/Api4/LocBlock.php
patching file Civi/Api4/Utils/CoreUtil.php
patching file ext/afform/admin/Civi/AfformAdmin/AfformAdminMeta.php
patching file ext/afform/admin/afformEntities/LocBlock.php
patching file ext/afform/admin/ang/afGuiEditor/afGuiEntity.component.js
Hunk #1 succeeded at 104 (offset -4 lines).
patching file ext/afform/admin/ang/afGuiEditor/elements/afGuiContainer.component.js
patching file ext/afform/admin/ang/afGuiEditor/elements/afGuiContainer.html
patching file ext/afform/core/Civi/Afform/Behavior/ContactAutofill.php
can't find file to patch at input line 379
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php b/ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php
|index c6b42fdb601e..557fc01ac721 100644
|--- a/ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php
|+++ b/ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php
--------------------------
File to patch: ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php
ext/afform/core/Civi/Afform/Behavior/GroupSubscription.php: No such file or directory

image

and on 5.78.1 I'm blocked a bit afterwards

patching file ext/afform/mock/tests/phpunit/api/v4/Afform/AfformUsageTestCase.php
patching file tests/phpunit/api/v4/Entity/LocBlockTest.php
can't find file to patch at input line 1256
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php b/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php
|index f5ed0e2af178..9e4e1b3f54a9 100644
|--- a/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php
|+++ b/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php
--------------------------

@colemanw
Copy link
Member Author

@allinappliadmin that file exists in the RC but not your version. I don't think it's critical to this PR so you could just ignore it.

@civicrm-builder retest this please

@allinappliadmin
Copy link
Contributor

should the test work on the 5.76.1?

@colemanw
Copy link
Member Author

@allinappliadmin I think so. One or more files will not patch, but that's probably ok.

@Guydn
Copy link

Guydn commented Aug 15, 2024

There is now a "block address" that is available in the event fields in FB. There are 3 main display possibilities: "autocompletion"," id. location", "select location form".

I prepared an autocompletion search (on entity address "with" address addresses "with" event location, "grouped by" id. address). When i select autocompletion, i try to select my search but I cannot see it and therefore cannot see my autocompletion display. I'm missing something. -> fixed ! :-) my search is now entity address (with a map) with address location, with event location.
There is a possibility to select a quick add but none are available and I do not see how I can create one.
Actually the global idea is very interesting :-): search in a list of addresses, select one and keep the option to create a new one through a quick add.

If I select "id. location", it is an integer. I do not see how this option can be used.

If i select "select location form", i do not understand how to use it. If I want to customize the options then the whole FB edit screen is frozen.

@colemanw
Copy link
Member Author

@Guydn I'm not entirely sure what you mean. Could you add a screenshot to illustrate what you mean by "quick add" and "id. location is an integer" and also "select location form". Thanks.

@Guydn
Copy link

Guydn commented Aug 15, 2024

Yes I get directly the french translation ...
Event location 3
Event location 4

The Id location
Event location 2

The select location form
Event location 6

@colemanw
Copy link
Member Author

@Guydn ok, I see the confusion. That field is "address_id" and it should not be there. I'm removing it. Sorry for the confusion.
The field you want is "Existing Location" - it is the first field in the list. Dragging a location block onto the form should now look like this:

image

@allinappliadmin
Copy link
Contributor

applying the patch on 5.76.1 leads to fatal error. Many files to be patched were lacking. Should I upgrade to RC (5.77)?

@colemanw
Copy link
Member Author

@allinappliadmin yes upgrading to the RC should help you apply the patch.

@allinappliadmin
Copy link
Contributor

@allinappliadmin yes upgrading to the RC should help you apply the patch.

Still having a missing file notice while patching the RC

|diff --git a/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php b/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php
|index f5ed0e2af178..9e4e1b3f54a9 100644
|--- a/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php
|+++ b/tools/extensions/phpstorm/Civi/PhpStorm/Api4Generator.php

@Guydn can you test and confirm it works as expected?

@colemanw
Copy link
Member Author

@allinappliadmin that file is not important. You can just delete it from the patch.

@Guydn
Copy link

Guydn commented Aug 20, 2024

I cannot input anything in the field "location Id" (5.77 beta1, WP 6.6.1)
CiviCRM Event location step 1
CiviCRM event location step 2
CiviCRM event location step 3

@Guydn
Copy link

Guydn commented Aug 21, 2024

On another environment (Civi 5.73, WP 6.6) managed by @mlutfy the patch works (good news !) but this patch was complex to implement.

@colemanw
Copy link
Member Author

Ok great, thanks @Guydn and @mlutfy. Indeed this is a big changeset, but it includes lots of test coverage so let's get this merged so you don't have to deal with complex patches anymore!

@colemanw colemanw merged commit b390236 into civicrm:master Aug 21, 2024
1 check passed
@colemanw colemanw deleted the afformLocBlock branch August 21, 2024 12:22
@Guydn
Copy link

Guydn commented Aug 22, 2024

Thanks a lot @colemanw it works well now on wpmaster :-) !

@Guydn
Copy link

Guydn commented Sep 16, 2024

I have discovered a issue (Wpmaster 5.79 Alpha1) that is critical to use the feature https://lab.civicrm.org/dev/core/-/issues/5465

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.

5 participants