custom field extends Phone Call instead of Inbound SMS message

[jmcclelland]

Somehow it still seems to work - perhaps due to a laxness on the part of the API.

Is there a reason for it?

Also, I'm curious if development of this extension is still active. I would like to help revive it.

[JohnFF]

Not actively active, but I'd accept a good pull request. It was never released as an extension, in spite of working really well and being really good.

[jmcclelland]

Thanks for the response. I'd like to put a bit of work on this.

This issue may be a good place to start.

I did more digging and I see that this is way more complicated then I originally thought.

Right now the "SMS Inbox Data" custom field set extends Activities of the type "Phone Call".

That means the "Message Read" field is exposed when you create or edit an Activity of the type "Phone Call" but not for an activity of the type "Inbound SMS".

I thought changing the custom field set to extend "Inbound SMS" would be a simple fix.

But, alas, it's not possible to have a custom field extend the "Inbound SMS" activity type - since that option value has filter set to 1 (the filter field in the civicrm_option_value table) . It seems that this filter controls both whether an activity type can be extended and also whether an activity type can be created from the New Activity menu. Since we don't want to let people create an "Inbound SMS" from the new activity window, changing this filter value won't work.

I suspect that's why it's currently set to "Phone Call" instead of "Inbound SMS" - because setting it to the logical value is not possible.

I think a better change - but one that is far more dramatic - is to eliminate this custom data set altogether and create a separate table for storing this data that is not exposed via the custom data section of the database. I think this approach both solves the problem at hand. And also it is a more elegant design. When someone uninstalls this extension, it should take all the data with it. But now, when you uninstall the extension it leaves the custom data set behind. And, since a custom data set could in theory be extended by a user to add more fields, it is dangerous to try to remove it.

However, if this info was handled in a custom table not exposed to the user, it would be safe to purge on uninstall.

What do you think? Would you be open to such a refactoring? And if so, would you require a migration path or, if it has never been released, would it be enough to simply fix this for new installs?

[JohnFF]

Hi jmcclelland,

From everything you've said, it sounds like what you've outlined is a good fallback approach, but isn't my primary choice for the following reason:

The root of the issue is not being able to extend fields on Inbound SMS activity. The reason for that seems to be the filter = 1. If we can't change that upon install, then we should go to the core team and establish the reason for it as a first port of call. But I'd be happy to change it upon install. A custom table has the disadvantage of (without extra work) not exposing its field. Other than that, it would be a good solution, and if we can't get filter = 1, then it would be a good fallback approach (although we lose the ability to see it in the interface pn the activity record).

[jmcclelland]

I did raise this in the CiviCRM Mattermost chat. I learned that filter=1 serves more then one purpose:

• It controls which activity types appear from the New Activity drop down (filter=1 prevents "Inbound SMS" from appearing in the New Activity drop down - which is the desired behavior)
• It controls which activity types can be extended (filter=1 prevents "Inbound SMS" from being extended - not the desired behavior)

Because the field is overloaded this way, simply changing filter=0 for "Inbound SMS" is not a great solution, either in core or in this extension. The right fix would be to add a second field in the option values table, but getting that approved upstream would be a real struggle.

[JohnFF]

OK, In light of the research you've done I agree that a custom table seems to be the best way forward. I also have the ability to approve extensions when they're done. Before doing this though, I would ask you to test it on all platforms. Let's take this offline and book in a zoom call for the next phase of development: john@civifirst.com

…

On Mon, 21 Dec 2020 at 13:44, Jamie McClelland ***@***.***> wrote: I did raise this in the CiviCRM Mattermost chat. I learned that filter=1 serves more then one purpose: - It controls which activity types appear from the New Activity drop down (filter=1 prevents "Inbound SMS" from appearing in the New Activity drop down - which is the desired behavior) - It controls which activity types can be extended (filter=1 prevents "Inbound SMS" from being extended - *not* the desired behavior) Because the field is overloaded this way, simply changing filter=0 for "Inbound SMS" is not a great solution, either in core or in this extension. The right fix would be to add a second field in the option values table, but getting that approved upstream would be a real struggle. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIXUSDBD3Y76ZCYS4J3YKDSV5GM7ANCNFSM4UXERBBQ> .

[jmcclelland]

I hope you don't mind that I went ahead with a PR rather then scheduling a video call. I found myself with some free time today and decided to go for it. I did a bit more then the refactoring of the state table - I had some trouble getting the SmsSender class to work so I tinkered a bit there.

Please let me know what you think and if you see any problems or things that could be improved.