snippet-library icon indicating copy to clipboard operation
snippet-library copied to clipboard

`gpnf-auto-attach-child-files`: Refactored for greater configuration flexibility.

Open veryspry opened this issue 2 years ago • 5 comments

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/2011825626/38759/

Summary

  • Refactors gpnf-auto-attach-child-files into a class.
  • Moves configuration logic to constructor args so that users can easily create multiple, unique configurations.
  • Adds child_form_upload_field_ids config option to limit which child form upload fields are included in a notification.

veryspry avatar Sep 28 '22 09:09 veryspry

I did a functional test and it looks good! I did notice that it slowed down submission a bit, but I'm not sure if that's an issue with the refactor itself.

Thanks for doing that! All was looking good during my tests but it's always nice to get an extra set of eyes on it :raised_hands:

As far as the slowdown goes, my guess is that it isn't due to the refactor as the logic is essentially the same as before. The main difference is the way that configuration options are set

If I had to guess why, database reads are probably the main culprit. For instance:

  • getting the form object which queries the form table and the form meta table: https://github.com/gravitywiz/snippet-library/pull/535/files#diff-6dc898ef07f19e6f8f6a1c641d2c979e25e78c5327ea4622eccf6b3eb35fb4c7R65
  • getting all entries for each child form: https://github.com/gravitywiz/snippet-library/pull/535/files#diff-6dc898ef07f19e6f8f6a1c641d2c979e25e78c5327ea4622eccf6b3eb35fb4c7R66

We also make a call to preg_replace() for each attachment we find which might impact speed noticeably?

That said, PCRE is supposed to cache compiled regex's which should be helpful since we pass the same regex string to each call of preg_replace()

https://stackoverflow.com/a/210086

veryspry avatar Sep 29 '22 09:09 veryspry

Just waiting on confirmation from a user that this works well for them before merging!

veryspry avatar Sep 29 '22 09:09 veryspry

Feature request:

Support for multiple child forms (in one parent form) whose upload fields have different id's.

For example: Parent Form has two child forms, A and B. A's file upload field has id of 1 and B's has a file upload field with id of 2.

I'm going to go ahead and add that functionality to this snippet before merging!

veryspry avatar Oct 03 '22 16:10 veryspry

@claygriffiths just a heads up that I changed the API here a bit so that this works with more complex child form configurations.

See here for more details: https://github.com/gravitywiz/snippet-library/pull/535#issuecomment-1265688832

veryspry avatar Oct 07 '22 19:10 veryspry

We're just waiting for a user to confirm this works for them

veryspry avatar Oct 14 '22 17:10 veryspry

This is confirmed to work as intended. I'm mergin' it!

veryspry avatar Nov 08 '22 16:11 veryspry