dev/core#4232 Afform: Allow manual processing of submission

[kurund]

This implements following functionality to FB:

• Save the data only as submission
• Manually process the submissions
• Ability to confirm submission via Email

[civibot]

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the 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

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[highfalutin]

I'm watching this work with interest, @kurund. Let me know i there's any testing to be done.

[kurund]

@colemanw

Ready for your review now.

Only thing missing is token evaluation in the email. It's bit tricky as we don't have contact.

[kurund]

I'm watching this work with interest, @kurund. Let me know i there's any testing to be done.

Most of the functionality should be working now expect token evaluation for submission verification. May be you can test after feedback from @colemanw

[colemanw]

@kurund the api_v4_AfformContactUsageTest failures are relevant.

[kurund]

@kurund the api_v4_AfformContactUsageTest failures are relevant.

Thanks, I have fixed the failing test.

[highfalutin]

@kurund thanks for all your work on this! I tried it out and everything looks good except the email token doesn't get processed in the message. Is that still WIP?

[kurund]

@highfalutin

Thank you. Yes, email token is only outstanding and needs to be fixed.

@colemanw

It would be good get your thoughts on email token processing.

[highfalutin]

I'm making an attempt to implement the email token. I'll post my code.

[highfalutin]

I've got the whole flow working on my fork; see this commit. Filling in the framework that @kurund created,

• The message token {afform.validateSubmissionUrl} is rendered into a url containing a JWT.
• That URL runs CRM_Afform_Page_Verify, which
  • decodes the JWT
  • gracefully handles expiration and other errors
  • if the JWT contains a valid pending submission id, processes the submission
  • shows a status message to the user

This is all good. However, I think that instead of just having a built-in status-message page as the endpoint of this workflow, we should redirect to the Post-Submit Page specified in the Afform's metadata. Perhaps admins should also be able to specify an Error Page — although we'd need to think through how to pass messages like "your token expired"

[colemanw]

That's great @highfalutin - based on your testing do you think this PR is mergeable & we can iterate on it in future PRs, or do you see any blockers to merging this?

@kurund I'm still seeing a test failure which is failing because of the 2 new tokens you added. I'm still not sure it's correct to be adding them as contact tokens since I don't think they expose contact data but rather afform-specific data, right?

[kurund]

@highfalutin

Thanks looks good to me. I have pulled your changes in this branch. I think we can revisit messaging at a later stage.

@kurund I'm still seeing a test failure which is failing because of the 2 new tokens you added. I'm still not sure it's correct to be adding them as contact tokens since I don't think they expose contact data but rather afform-specific data, right?

Yes, they are afform submission specific tokens. I don't think the token are added as contact specific.

[kurund]

@highfalutin

I have made minor fix to generate the verify URL for the frontend.

[kurund]

@highfalutin @colemanw

As discussed over MM I have moved the token registration to Symphony event.

[highfalutin]

@kurund it looks great. And passes tests on my local environment. @colemanw do we have a way of loudly marking something as experimental? The interface for this feature may be changing significantly, and people should be aware of that.

[colemanw]

@kurund looking at the code in this afformSubmissionProcessTask.html and the related angular controllers/modules, it looks to me like all it does is make one api call with no user-configurable params. In that case, I think this could all be simplified as an apiBatch. You could delete the new angular module/js/html and just replace this line with:

Suggested change

	'uiDialog' => ['templateUrl' => '~/afSearchTasks/afformSubmissionProcessTask.html'],
	'apiBatch' => [
	'action' => 'process',
	'params' => NULL,
	'confirmMsg' => E::ts('Confirm processing %1 %2?'),
	'runMsg' => E::ts('Processing %1 %2...'),
	'successMsg' => E::ts('Successfully processed %1 %2.'),
	'errorMsg' => E::ts('An error occurred while attempting to process %1 %2.'),
	],

Hmm, but now that I look at it, it seems the process action has been added to the Afform entity. The above would assume that the action belongs to the AfformSubmission entity. Could we move the action over there and convert it to a use AbstractBatchAction? Then the above would know how to handle it, and an added benefit is that the api would be able to process more than one submission at a time.

[kurund]

@colemanw

process was added in Afform entity as it reuses code in Civi\Api4\Action\Afform\AbsctractProcessor which already handles the saving of submitted data.

Moving it to AfformSubmission might be bit tricky.

Thoughts?

[colemanw]

Perhaps we could leave Afform::process where it is, but also add an AfformSubmission::changeStatus action which is the bulk action for processing 1 or more Afforms (and it delegates work to Afform::process. This way the admin could view submissions, select a bunch of checkboxes and bulk process them (or bulk reject them, which might be useful for spam).

[kurund]

@colemanw

Bulk action is already available and working as expected. So admin can process at entry level or via batch.

[colemanw]

I finally figured out how to do this using chaining:
#31906

[kurund]

@colemanw

re: test failures, I am not sure why submission tokens are evaluated while asserting other component tokens. I am having a weird issue running PHPUnit 9 on local. What's the recommended PHPUnit version?

@highfalutin

Do you see any test failures on your local?

[colemanw]

@kurund my test configuration in PHPStorm looks like this:

[highfalutin]

@kurund

I am not sure why submission tokens are evaluated while asserting other component tokens.

I'll run some of the tests locally now and see if I can figure it out.

[kurund]

@eileenmcnaughton

Sorry about the delay looking at this

No worries, thanks for the review.

I see one fail on testGetModules which I can't comment on

Civi\Angular\ManagerTest::testGetModules Failed asserting that an array does not have the key 'settings'.

/home/homer/buildkit/build/build-4/web/sites/all/modules/civicrm/tests/phpunit/Civi/Angular/ManagerTest.php:81 /home/homer/buildkit/build/build-4/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:242 /home/homer/buildkit/extern/phpunit9/phpunit9.phar:2307

I have fixed this. There were few changes in the way SK tasks defined.

And the token one which I think is what you want me to comment on

CRM_Core_BAO_MessageTemplateTest::testContactTokens Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ '{contact.checksum}' => 'Checksum' '{contact.id}' => 'Contact ID' '{important_stuff.favourite_emoticon}' => 'Best coolest emoticon'

• '{afformSubmission.validateSubmissionUrl}' => 'Validate Submission URL'
• '{afformSubmission.validateSubmissionLink}' => 'Validate Submission (Full Hyperlink)'
  )

/home/homer/buildkit/build/build-4/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/BAO/MessageTemplateTest.php:528 /home/homer/buildkit/build/build-4/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:242 /home/homer/buildkit/extern/phpunit9/phpunit9.phar:2307

Reading the comments it seems like the goal is that the new tokens are always offered when rendering contact tokens - this seems a little confusing to me - however I'm not deep enough into this to be sure if that is a problem. It indeed the intent is for the tokens to be broadly available and given this extension is always enabled in core I don't think it would be too big a deal just to alter the tests to expect it. Sure it is a bit grey on core vs ext but not the only place that line is not respected

The alternative is to add some additional schema context to the list tokens call & the list function can check for that

I have updated the test to unset these special submission tokens.

Hopefully now the tests should pass .

[kurund]

@colemanw @eileenmcnaughton @highfalutin

Previously failing tests are passed and the current failure: https://test.civicrm.org/job/CiviCRM-Core-Matrix-PR/7256/BKPROF=dfl,SUITES=mixin,label=bknix-tmp/console is not related.

So I guess ready for merge :)

[mattwire]

Go on then @kurund you deserve it!

[kurund]

thanks @mattwire

[highfalutin]

Congratulations on getting this over the finish line, @kurund! And thanks to those who helped.