Extension review

[colemanw]

I've run this extension and looked at the code. In general it looks very well written and well-documented. This pr addresses some extremely minor code style issues and adds a necessary dependency to the info file.

In approving this for automated distribution I have one concern: the function CRM_Jsumfields_APIWrapperSumfieldsGendata appears to be running on every api call(!) and calling a function (_jsumfields_generate_data_based_on_current_data) which says it is "designed to be run once when the extension is installed or initialized." What's going on there?

[colemanw]

Update: I see that api wrapper is only being invoked when the entity is "sumfields" and the action is "gendata" and that makes me rest easier. But I'm still confused by the comment claiming that it only runs on install.

[twomice]

Thanks for the PR and the comments, @colemanw You wrote

comment claiming that it only runs on install.

I can't seem to find that comment. Can you link to where you're seeing it?

[colemanw]

https://github.com/twomice/com.joineryhq.jsumfields/blob/master/jsumfields.php#L2725-L2726

[twomice]

@colemanw I see. Thanks. It says "installed or initialized" and then doesn't explain anywhere what is meant by "initialized". From memory, I think it's referring to the kind of "initializing" that's hinted at under jsumfields_update_sql in https://github.com/twomice/com.joineryhq.jsumfields/blob/master/DEVNOTES.md. I agree it could use a better explanation and is confusing/misleading as-is.

[colemanw]

Ok, happy to give this extension an "Approved" status once these little things are fixed.

[twomice]

Thanks for the PR, @colemanw . I've merged it.

As for documentation on _jsumfields_generate_data_based_on_current_data(), I've edited the docblock to be a little more explicit; see https://github.com/twomice/com.joineryhq.jsumfields/commit/5e14268d4badc38adece5cb5d8f2552353d3bd8e

[colemanw]

Depending on how explicit, it may need this sticker.