Overview

Ive spent a some time analysing the sms_user module over the last 2 days, especially in regards to how phone number fields and sleep fit in. This issue expands on, and is parent of, #2606290: Implement SMS User information as a Drupal 8 Field

Ive determined there isnt actually any need to strictly couple with user entity in any way, and therefore functions described in this document should be implemented in the main sms module.

However one feature should be kept in a seperate module, as it is user specific: Registrations via SMS. This is due to the possibility to include a password with the registration request. Registration via SMS is a whole can of worms since there are many configuration options.

Other functions of sms_user can be moved to the main module as the functions they provide arn't necessarily unique to 'Users', and instead can be abstracted out to 'Entities'. There should be an automated 'wizard' that automates association of an entity type and creates a phone field, or provides capability to target existing phone field.

This is a large undertaking, I am willing to implement it.


Features

  • Add a service to determine phone number(s) for an entity:
    $number = \Drupal::service('foobar')->getPhoneNumber($entity)
  • Add dependency on the following modules, read on to see reasoning:
    • telephone (in core)
    • dynamic_entity_reference (almost made it into core, considered for 8.1)

Global settings

Settings, configuration entities, and content entities.

Phone number verification

New feature.

Allow site to specify whether entity must verify his phone number before SMS are allowed to be sent.

Boolean (checkbox), site default: ON/TRUE (requires number verification)

Entity

Configuration

The following config schema is created to associate entities with phone numbers. When the term 'entity' is used, you can safely replace with 'user'. Terms/variable names are NOT final:

For example, the user entity config entity ID is sms.phone.user.user.

Summary in YAML format

sms.phone.{entity_type}.{bundle}:
  mapping:
    allow_automated_optout
      type: boolean
    message_verfication
      type: string
    duration_verification_code_expire
      type: int
    field_id_phonenumber
      type: string
    field_id_automated_message_optout
      type: string

Descriptions

  • allow_automated_optout
    Allow entities to opt out of "automated messages". This replaces the existing opt-out setting which blocks all messages. See "Automated Messages" section.
  • duration_verification_code_expire
    Amount of time (in seconds) before verification code expires.
  • message_verification
    Message sent to number to verify account association. No change from existing functions.
  • field_id_phonenumber
    ID of phone number field attached to entity. See "Fields" below.
  • field_id_automated_message_optout
    ID of checkbox field attached to entity. See "Fields" below.

Fields

The following fields [are/need to be] created on the entity with phone number:

  • Phone number (:field_id_phonenumber)
    field type: telephone
  • Automated message optout (:field_id_automated_message_optout)
    field type: checkbox

Verification status

Stores verification state for entities. Verification status is considered state, therefore it should not be stored on the entity. In addition, verification status is not useful in integrating with systems such as Views.

If verification status is unverified after expiration date passes, then delete the verification.

Entity definition

  • Reference to entity
    type: dynamic_entity_reference
  • Verification code
    type: string
    Use string instead of int, permitting other types of verification codes. See "Verification Code System".
  • Verification status
    type: boolean
    Constants: FALSE (NOT VERIFIED); TRUE (VERIFIED)
  • Verification code expiration date
    type: date
    Expires the verification row, prevents guess attempts over long time.

Verification Code System

Provide a code generator plugin? system, allowing site admin to choose other codes than 1000-9999. The chosen code generator should be specified in sms.phone.{entity_type}.{bundle} config.

Automated Messages & Sleep

Add the following info to SmsMessage:

var $is_automated boolean, default value: true

If the message was generated automatically. This variable is used when determining whether the message should be sent outside of sleep hours.

For example: if a user personally triggers a SMS message, such as registration, then the SMS should be sent ASAP. Whereas if the message was generated as a result of a script, or in bulk, then the SMS message should be added to a queue to send later.

The :field_id_automated_message_optout field on the entity is also used when determining if an automatic message can be sent outside of sleep hours.

CommentFileSizeAuthor
#57 interdiff.txt1.89 KBdpi
#57 sms-entity-phonenumber-2643692-57.patch120.8 KBdpi
#52 Screen Shot 2016-02-16 at 11.44.32 AM.png31.91 KBalmaudoh
#52 Screen Shot 2016-02-16 at 11.43.40 AM.png26.99 KBalmaudoh
#50 interdiff.txt2.03 KBdpi
#49 sms-entity-phonenumber-2643692-49.patch121.22 KBdpi
#47 sms-entity-phonenumber-2643692-47.patch120.77 KBalmaudoh
#47 interdiff.txt5.43 KBalmaudoh
#37 sms-entity-phonenumber-2643692-37.patch120.75 KBalmaudoh
#37 interdiff.txt31.38 KBalmaudoh
#36 sms-entity-phonenumber-2643692-36.patch121.27 KBdpi
#36 interdiff.txt9.33 KBdpi
#35 sms-entity-phonenumber-2643692-35.patch120.64 KBdpi
#21 sms-entity-phonenumber-2643692-20.patch120.61 KBdpi
#20 sms-entity-phonenumber-2643692-20.patch138.6 KBdpi
#20 interdiff.txt58.15 KBdpi
#16 sms-entity-phonenumber-2643692-16.patch71.68 KBdpi
#16 interdiff.txt5.63 KBdpi
#10 sms-entity-phonenumber-2643692.patch69.69 KBdpi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi created an issue. See original summary.

dpi’s picture

Issue summary: View changes
dpi’s picture

dpi’s picture

I have some spare time over the weekend I'd love to allocate to this. Let me know if you have any opinions. I'm on IRC whenever I'm awake.

almaudoh’s picture

A good way to go about this would be to build a parallel system independent of the sms_user.module implementation which allows us to implement incrementally these features and then when it's all done, we can remove the sms_user implementations. #2606290: Implement SMS User information as a Drupal 8 Field is a good place to start.

In addition, the Phone number verification system should be implemented as a service.

dpi’s picture

I agree, once we have ported most features to the main sms.module, we can remove duplicate code from sms_user.module.

Awaiting green light from you; regarding all points made in issue summary.

Side note: Cant start for at least 24 hours, dealing with catastrophic database loss and related issues.

almaudoh’s picture

Sorry to hear about the database loss. You can go ahead with the redesign. I suggest a child issue for each separate logical (or modular) piece of this, viz.:

Or something in those lines.

Edit: corrected the wrong issue link

dpi’s picture

Can do

dpi’s picture

dpi’s picture

Here it is.

If you can do user workflow + UI testing and code review, I'd appreciate it.

I have not added any new tests as I want to make sure we are on the same page about the aforementioned.

Summary of changes incoming...

dpi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: sms-entity-phonenumber-2643692.patch, failed testing.

The last submitted patch, 10: sms-entity-phonenumber-2643692.patch, failed testing.

dpi’s picture

Dont mind the test fails. Its due to project dependency change. The issue can be resolved by committing a dependency-only change to HEAD.

dpi’s picture

Patch summary

The following have been addressed in the patch

  • Added verification and phone number settings entities, with getters and setters.
  • Associating an existing, or automatic creation of a new, telephone field with an entity.
  • Added phone number service. Featuring:
    • method to get phone numbers for an entity
    • method to send sms message to an entity.
  • Added PhoneNumberSettingsException for when code expects a result, but is not available.
  • Phone number settings form includes inline AJAX form reload when *adding new* phone number settings.
  • Added aggregate phone number verification statistics on phone number settings list.
  • Sitewide verification form, with flood protection.
  • Users dont have to be logged in to use verification code (still need to give anonymous users permission). This means a user can click the verify link on his phone, and enter the code.
  • Expired verification codes will be cleaned up by cron, and optionally phone number field values can be purged at the same time.
  • Added telephone field form widget: shows information about phone number verification.
  • Telephone fields can have any cardinality (multiple phone numbers per entity). Designed to, and fully tested to work.
  • Fixed SMS message tokens.
  • Improved token list fallback, for when token.module not available.

The following need to be addressed in this issue:

  • Add tests (within this same issue)

And these as direct follow up:

  • Phone number validation
  • Add configuration path for phone number verification form (currently /verify)
  • Implement sleep functions (automated message preference)
  • SMS message transmission as a FIFO queue.
  • removal of duplicated functions in sms user

Setup instructions:

  • Re-install module. This is due to new entity type.
  • Only need to enable sms.module. Submodules not required.

Configuration

Setup

  • Go to Admin -> Configuration -> SMS Framework: Phone numbers
  • Click 'Add phone number settings' local action.
  • On the 'Add phone number settings' form:
    • Bundle: User
    • Phone number: - create new field -
  • Click 'Save' button.

User settings

  • Go to a user profile edit form. (eg: user/{user}/edit):
  • Enter a value into 'Phone number' text field.
  • Click 'Save' button
  • Take note: when going back to the same profile edit form, the form widget will change.
  • At this point, a SMS will be sent to the phone number entered in the text field. (Check recent log messages if using logger gateway).

Phone number validation

  • Go to http://yoursite.com/verify and enter the code you received in the SMS message.
  • Again take note: after validating a phone number, the form widget will change on the profile edit form.
  • Once the phone number has been validated, the following will operate correctly:
    • $numbers = \Drupal::service('sms.phone_number')->getPhoneNumbers($entity)
    • $result = \Drupal::service('sms.phone_number')->sendMessage($entity, $sms_message)
dpi’s picture

Changed status since testbot results are useless.

Changes since #10:

  • getPhoneNumbers now checks (by default) for verified phone numbers.
  • Moved SmsException base exception class to Exception subdirectory.
  • Added NoPhoneNumberException for if code incorrectly assumes an entity has a phone number.
almaudoh’s picture

Assigned: dpi » almaudoh

I'm taking a look at this...

dpi’s picture

No problems, just trying to figure our a schedule for when I can follow up on your critiques

almaudoh’s picture

I should be able to get through it before the weekend.

dpi’s picture

Adding a lot of tests :)

And fixing issues uncovered by tests.

dpi’s picture

Status: Active » Needs review
FileSize
120.61 KB

Lets try again.

Same idiff

Status: Needs review » Needs work

The last submitted patch, 21: sms-entity-phonenumber-2643692-20.patch, failed testing.

The last submitted patch, 21: sms-entity-phonenumber-2643692-20.patch, failed testing.

dpi’s picture

They're all passes on my local machine due to having dependencies.

Drupal\sms\Tests\SmsFrameworkPhoneNumberAdminTest             98 passes                           28 messages
Drupal\sms\Tests\SmsFrameworkPhoneNumberTest                  12 passes
Drupal\sms\Tests\SmsFrameworkPhoneNumberVerifyForm            58 passes                           16 messages
Drupal\sms\Tests\SmsFrameworkPhoneNumberWidgetTest            65 passes                           19 messages
Drupal\sms\Tests\SmsFrameworkWebTest                          73 passes                           14 messages
Drupal\Tests\sms\Kernel\SmsFrameworkPhoneNumberProviderTest   12 passes
Drupal\Tests\sms\Kernel\SmsFrameworkTokenTest                  3 passes
Drupal\Tests\sms\Kernel\SmsFrameworkVerificationMaintenanceT   5 passes

I have been in contact with batje, and have obtained co-maintainer status. I'd like to get in contact with you and discuss things rather than going on my own. Unfortunately I have not seen an email reply from you for months...

almaudoh’s picture

Congratulations on the co-maintainer status. I can see your passion for the SMS Framework already. Hope we can work together as a team.

Unfortunately I have not seen an email reply from you for months...

That's an exaggeration... I've been away (from serious coding) for about 15days or so. I'm still reviewing the rather large patch. And would make the more detailed review comments shortly.

Can you split the patch into 3 components per #7 for easier review...

dpi’s picture

Im sorry, i didnt interpret from #7 that you wanted a separate patch. From #7, I have not implemented any of "Automated messages and sleep-time messaging" in this issue, and the first two bullets are closely integrated because of the widget. I find it difficult or not logical to separate the two.

That's an exaggeration...

I have not sent you email this year. I mean I sent you email in September and November with no responses. At this point the emails I sent don't matter.

almaudoh’s picture

I have not sent you email this year. I mean I sent you email

Ok, you were talking specifically of email...I will respond going forward...what is the best time for you (UTC) to chat on IRC?

dpi’s picture

Exact times are unpredictable, but at least 50% of each day. I am on IRC when I'm working and at home. In any case, my IRC bouncer is always connected.

dpi’s picture

We are currently using patchflow for SMS Framework, perhaps we can make use of Github for review purposes. I'm not sure how familiar you are with Github PR so ill just outline how we can take advantage of it. I have created a dummy PR on Github so you can annotate lines if you want. As opposed to copying lines into D.o issue.

  1. Login to Github
  2. Go to https://github.com/dpi/smsframework/pull/2/files
  3. Hover over lines that you want to criticize, click blue '+' symbol.
  4. Write comment
  5. Repeat 3-4 as necessary
almaudoh’s picture

I actually use dreditor (dreditor.org) for code reviews which makes it easier, so I don't have to copy lines into the issue comment.
Edit: I will check your git repo

dpi’s picture

Awesome, just trying to lubricate the gears.

almaudoh’s picture

I've finally had some time to look through the code (excluding the tests and also not tested manually) and I've made some comments on the git repo. Here's a summary

Decoupling Phone number verification and status

This makes sense in its entirety and I'm fully okay with that. Can we have Views integration so an admin can build a view of phone number verification status (maybe separate issue). I noticed the phone number settings list builder primary only holds information on phone number verifications, so maybe the name should be PhoneNumberVerificationListBuilder then...

Phone number field

Again, the concept also is great. However, the implementation is where there are concerns on complexity. Maybe there's no simpler way to do it, I don't know, but I left a comment on that also:

I understand this approach, but I'm still just wondering if we couldn't have achieved a simpler implementation by extending the Drupal core phonenumber field and implementing the additional functionality there? Here we've ended up creating a whole new config entity type just to manage specific settings which could be encapsulated by the field.

So a whole additional config entity with the accompanying classes (config entity class, interface, add/edit form, delete form, list builder, etc) just for some custom settings which could be incorporated in the field itself...

Phone number SMS provider

This is also a nice decoupled implementation, only thing is that the purpose is not clear from the naming of the provider (sounds like it provides phone numbers :-)).

I've not looked at the tests yet. But I've commited the .info.yml file with updated dependencies to allow tests to pass on D8 testbots.

Great work!!!

  • almaudoh committed dc49f54 on
    Issue #2643692 by dpi: SMS User Redesign. Committed test dependencies...
dpi’s picture

Can we have Views integration

Yeh we can do this in a seperate issue.

I noticed the phone number settings list builder primary only holds information on phone number verifications, so maybe the name should be PhoneNumberVerificationListBuilder then...

The statistics on the list are only supplementary to help the administrator get a birds eye view on pending verification. Its like if the node type list had statistics on how many nodes existed with that type. In the end, the purpose of PhoneNumberSettingsListBuilder is to display a list of phone number setting config entities. Keeping with existing conventions of prefixing camelcase entity type onto ListBuilder.

Phone number field vs phone number verification entity

If possible, I have a preference of tacking functionality on top of existing primitive types, rather than inventing a proprietary field type.

We make use of a standard telephone field instead of custom field, this lets us use telephone field data that existed before SMSFwk is installed. And lets users uninstall SMSFwk without having to delete a proprietary field type we invented. Ff we were using a custom field, there would be no need to target existing field, we may as well have hard coded a field name into each entity type.

It is on a separate entity because if we were to store verification state on a field, and when we come around to purge verifications from an entity. And the entity for some reason no longer validates (due to numerous reasons, outside of SMS Framework even) then we cannot save the new verification status, because the entity will not save without being valid (the Drupal 8 entity validation system is complicated...). This is also partially the reason why the 'purge phone number from entity' option is available, because it may not make sense to edit each entity in bulk. Saving may cascade problems to other systems, and in the end the entity may not even save.

You're right it does add some complexity to the system, but I fear we would also have an imperfect and inflexible system with custom field types.

only thing is that the purpose is not clear from the naming of the provider (sounds like it provides phone numbers :-)).

One of the most difficult things in programming is naming things. Technically it does provide phone numbers. But also the verification system around it. Yes it sounds a little wonky, but the simple name grew on me. I'd love to hear better suggestions.

I've not looked at the tests yet.

Tests dont need a thorough review, if they turn out wrong they can be corrected. During test creation I uncovered many issues, which were fixed in the same patch.

dpi’s picture

Rebase on head. no idiff

dpi’s picture

almaudoh’s picture

I had put together some write-up to go with this, but somehow I lost it. I'll take sometime to write it up again. Here's just some docs cleanup and naming changes. Hope I didn't break anything... :-)

Status: Needs review » Needs work

The last submitted patch, 37: sms-entity-phonenumber-2643692-37.patch, failed testing.

The last submitted patch, 37: sms-entity-phonenumber-2643692-37.patch, failed testing.

dpi’s picture

I'm on the road at the moment, but looking at the interdiff I can see case change. We actually use underscore case, not upper/lower camel case for entity properties.

Same with is/can

dpi’s picture

Apparently Dreditor no longer works with Firefox, so I'll continue with Github PR review:

https://github.com/dpi/smsframework/pull/3
https://github.com/dpi/smsframework/pull/3/files

Tests need to be fixed.

Mostly good changes, but entity property to camelCase and method is->can prefix need to be reverted because they are against Drupal conventions.

almaudoh’s picture

Apparently Dreditor no longer works with Firefox

Yeah, that has to do with Firefox's new policy of signing scripts...unfortunately, there's no solution very soon. I use Google chrome more often though and Dreditor still works fine.

...entity property to camelCase and method is->can prefix need to be reverted because they are against Drupal conventions.

I checked drupal coding standards (https://www.drupal.org/node/608152#naming), camelCase is not against Drupal conventions, just that underscores are allowed (I'm shocked to find that clause even exists, because all along the talk had been changing underscores to camelCase). We can revert those hunks, but the names should be concise and descriptive.

almaudoh’s picture

Made some comments on the PR, ahttps://github.com/dpi/smsframework/pull/3/files. Dunno why the tests are failing. Looking into it now.

dpi’s picture

It is not against the "rules", but it is a "convention". As defined: "a way in which something is usually done.", as evident by looking through Drupal code, especially in regard to entities.

because all along the talk had been changing underscores to camelCase

I dont know about this...

but the names should be concise and descriptive.

I dont have an issue (mostly, other than a prefix change) with the way the variables were renamed.

Dunno why the tests are failing. Looking into it now.

I'm sure many fails are due to the property name mismatch. Try reverting the camelCase properties and re-run tests.

Are you running the code locally? The test are very practical, fails would likely surface during manual UI-only usage.

almaudoh’s picture

because all along the talk had been changing underscores to camelCase
I dont know about this...

Check the meta-issue in views #2052421: [META] Rename Views properties to core standards

dpi’s picture

Check the meta-issue in views #2052421: [META] Rename Views properties to core standards

Variables mentioned are all temporary state or injected services. None are entities. Also just for Views, which is a mess.

almaudoh’s picture

Ok. Here's a revert of the config names. If this passes, then should be RTBC.

Status: Needs review » Needs work

The last submitted patch, 47: sms-entity-phonenumber-2643692-47.patch, failed testing.

dpi’s picture

Fixed phone number purging.

Was using variable name before it was renamed.
Added extra assertion to make it easier to debug in the future.

dpi’s picture

FileSize
2.03 KB

Forgot interdiff

Its just the contents of 12ee78db

almaudoh’s picture

Here's some things we should do in follow ups:
1. Create a list of verifications (or maybe views integration) for admin.
2. The "verify your number" work flow UX needs to be improved. At the very least a destination parameter to return back to the form after verification.
3. The entity view immediately displays a phone number that hasn't yet been verified. It shouldn't, or at least it should mark the number unverified.

Why I've not RTBC'd yet is because of one test failure in my localhost simpletest UI, trying to figure out why.

almaudoh’s picture

Edit 1: It seems the calculation of verification lifetime is off by 1 minute on my machine. Can't figure out why...

Edit 2: Embedded images

dpi’s picture

1. Create a list of verifications (or maybe views integration) for admin.

Hmm... Not sure this is a core feature. Like, why does anyone other than the user need to know about status.

2. The "verify your number" work flow UX needs to be improved. At the very least a destination parameter to return back to the form after verification.

Once you have verified the phone number, what is the point in returning the user to the edit form? UX question #1: Why does a user care?

3. The entity view immediately displays a phone number that hasn't yet been verified. It shouldn't, or at least it should mark the number unverified.

I suppose we can add a custom field formatter. Although I question how often a site builder will make the phone number publicly visible in the first place.

Why I've not RTBC'd yet is because of one test failure in my localhost simpletest UI, trying to figure out why.

Probably due to kernel tests, they are very fragile in Testing UI. (Use CLI)

dpi’s picture

Edit 1: It seems the calculation of verification lifetime is off by 1 minute on my machine. Can't figure out why...

I see, What test/line number is it?

almaudoh’s picture

SmsFrameworkPhoneNumberWidgetTest line 68

dpi’s picture

In retrospect, I realized the execution of tests with time calculations is unreliable on slower runners. Ill update the tests.

dpi’s picture

almaudoh’s picture

Status: Needs review » Reviewed & tested by the community

n retrospect, I realized the execution of tests with time calculations is unreliable on slower runners

The trick is to write it with the expectation that it may have to run slow in certain circumstances, otherwise you end up with random test failures. However, it would have been nice to have a test to ensure that the time is counting downwards :).

However, that shouldn't hold up the patch. RTBC!!

Great work @dpi, you may want to commit this yourself seeing as you have maintainer status.

almaudoh’s picture

Issue summary: View changes
dpi’s picture

The trick is to write it with the expectation that it may have to run slow in certain circumstances, otherwise you end up with random test failures.

I wrongly assumed that the test runner force set request time for pages loaded in the internal browser. It only does this between tests.

However, it would have been nice to have a test to ensure that the time is counting downwards :).

Sometimes we cant have nice things.

Ill commit this later today

  • dpi committed 4b8c756 on 8.x-1.x
    Issue #2643692 by dpi, almaudoh: Merged functions of SMS User into main...

The last submitted patch, 47: sms-entity-phonenumber-2643692-47.patch, failed testing.

The last submitted patch, 16: sms-entity-phonenumber-2643692-16.patch, failed testing.

The last submitted patch, 16: sms-entity-phonenumber-2643692-16.patch, failed testing.

almaudoh’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.