Problem/Motivation

The current implementation of the sms_user data attached to a user profile is as an undeclared class property.

Proposed resolution

Implement sms_user data as a Drupal 8 field which can be attached to the User entity.
Attach the sms_user field by default to the User entity using hook_entity_base_field_info() to retain present functionality.

Remaining tasks

Patch
Reviews
Commit

User interface changes

None

API changes

TBD

Data model changes

The sms_user database table will be removed and replaced by the D8 auto generated field tables for sms_user field type.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

almaudoh created an issue. See original summary.

dpi’s picture

Assigned: Unassigned » dpi

I can work on this, are you OK with sms_user submodule relying on the Telephone module bundled with Drupal 8?

Or did you have an alternate plan in mind?

dpi’s picture

I think it may be worth removing the fake $user property while I'm at it.

almaudoh’s picture

FileSize
19.13 KB

I had started some work on this. Maybe I'll just post the patch here so you can continue. I was planning to actually make the fake sms_user property into a D8 field.

Since, you're taking this on, here's where I am at right now. Feel free to go with it or start something different. I'll review.

dpi’s picture

I'm yet to take a look at your patch, but these are some thoughts I had in the time between #3-4:

  • Allow any entity type - decouple from the user entity type.
  • Ensure unique phone number across attached bundle /or/ entity type /or/ global (inspired by #1363942: Enforce unique phone numbers)
  • site admin can choose to target phone number field at existing field on bundle, or automatically create a telephone field and attach it.
  • Since the field system allows for multiple values, allow multiple phone numbers per entity (user). Inspired by #2606520: Can I have multiple mobile phones per user?. Technically, the sms service will try each phone number attached to the entity until an SMS is successfully reported as sent.
  • Add service to determine phone number(s) for an arbitrary entity (user)

Let me know what you think about these design changes. These dont necessarily need to be covered by this patch, but they are very relevant.

almaudoh’s picture

@dpi: all your points look good to me. I would only add that by default, we should attach the new field to the user entity when sms_user module is installed to maintain some backward compatibility with existing code (tried to do that in my patch).

Additionally, some of the other that should be incorporated to the phone number field:

  • The phone number status (whether confirmed or not)
  • The verification code

However, the fields relating to sleep settings and opt-in to SMS messages should probably be implemented as separate fields (maybe re-using existing field types in D8 core).

dpi’s picture

Ah, I've taken a look at your patch.

Adding properties (status, code, gateway etc) to a single field may be overloading the field system. The downside to using this method is SMS Framework needs to implements its own widget, formatter, and Views integration for the field. In addition, custom field formatters and widgets need to be written in order to create custom displays of the data.

My proposal was to add Drupal primitive fields to the bundle, the same as adding fields to a bundle using Field UI. SMS Framework only needs to initially create the field instance and attach it. Then maintain which field the relevant data is stored. This way any existing formatters and widgets from can be used.

For example, Telephone field:

  1. Create a telephone field
  2. Attach the field to entity/bundle: user/user
  3. Store the ID of the telephone instance as SMS config.

Create a field and instance, maintain association as config for each:

  • telephone Number
  • boolean Status (aka verified)
  • string Code
  • entity_reference Gateway (reference the config entity)
  • boolean Sleep enabled
  • time sleep_start
  • time sleep_end
  • boolean Opt out?
dpi’s picture

Sleep

I'd also like to add that perhaps the sleep system could be handled on a site level?

For example a site-global setting to only send SMS between time_start and time_end depending on users local time. SMS Framework can determine the Country (from the phone number) and/or timezone of the user, then localize the current time of the user.

I'm not sure if users actually want to micro-manage this kind of preference? Site admins should be responsible enough to not send SMS at inappropriate hours; a sensible default should be to enable the sleep functions with default range of 9am-5pm (which automatically adjusts to user local time). If at a later time a site actually needs user micromanagement; it shouldnt be much effort to create a third party module which hooks into SMSFwk and delays delivery.

Confirmation code

Code is only useful as temporary state. It may be better to maintain code as its own table row, allowing the cron system to clean up old entries more efficienty

dpi’s picture

Any opinions on above?

almaudoh’s picture

#8:
Sleep: A good compromise I think is to have a system / global level setting like you suggest, but also allow users to override those settings if they need to.

Confirmation: A separate table makes sense too. Maybe even a separate service to handle the whole phone number confirmation business, that way we decouple the two concerns.

#7: I agree in principle. We don't need the EntityReference (gateway) field though as that simply complicates matters and is hardly used by anyone AFAIK.

dpi’s picture

and is hardly used by anyone AFAIK.

Search for BaseFieldDefinition::create('entity_reference') in Drupal.

entity_reference is used by entities that reference a bundle. for example: the 'type' field on node entities is an entity_reference. There is less overhead with ER than D7, and it is included as a *core* field type. (There is no longer a Entity Reference module)

almaudoh’s picture

and is hardly used by anyone AFAIK.

I meant the "gateway" part of $user->sms_user...

dpi’s picture

Ah ok, interpreted that incorrectly.

We can remove it ;)

dpi’s picture

Ill tackle this issue as soon as #2642024: Gateway instances as config entities is resolved, as it is rather monolithic.

dpi’s picture

dpi’s picture

dpi’s picture

Dang wrong issue

almaudoh’s picture

Status: Active » Closed (duplicate)

The scope of this issue has already been done in the parent issue #2643692: SMS User Redesign