Add a base for users to subscribe to digests and store that data.

Proposed resolution :
Add a field to the user entity which allows them to select any active digests and subscribe.

Alternative resolutions :

Utilise content entities to hold a one to one relationship between a digest and a user. See #2
Utilise content entities to create a one to many relationship between a digest and users. See #5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DerekCresswell created an issue. See original summary.

DerekCresswell’s picture

FileSize
21.38 KB

There is going to be a good chunk more work done on issue 3152692 first so I will be leaving this on the back burner for now.

Posting the patch mainly for storage. It does not need to be reviewed yet as it is just a WIP.

DerekCresswell’s picture

Just going to leave this here as it helped to solve a problem with one of the tests, we need to check if there was a result returned from the subscription query otherwise errors are generated.

      // Check if a result was returned.
      if (current($subscription_id)) {
        /** @var \Drupal\digest\Entity\SubscriptionInterface $subscription_entity */
        $subscription_entity = $subscription_storage->load(current($subscription_id));
        $entity_exists = isset($subscription_entity);
      }
      else {
        $entity_exists = FALSE;
      }
DerekCresswell’s picture

Status: Active » Needs review

Here is a subscription entity. I've not added any forms for subscribing to digests here to keep the patches small. I'll make a separate issue for that soon.

Both the digest of the subscription and the users are kept as entity references. I've decided to use a one-to-many relationship model since that is what a digest and users is in real life.

DerekCresswell’s picture

FileSize
10.03 KB

If I attach the patch that is. Haha

travis-bradbury’s picture

FileSize
42.78 KB

I don't get the choice to make one Subscription entity for every Digest entity and use a field for the user reference. They should be config entities or content entities, not both. I tried to visualize this to see if my understanding is correct. I think we have this first one: a Subscription entity, plus a user reference field, which gives us a table that maps digests to users.

some digest entity options

https://docs.google.com/drawings/d/19LUgUxNL0NIQAZeAB7rwBpLkJU4OYI4lSm1B...

The other two are other options that avoid duplicating digests as configuration and content. The middle one is a "digest_subscription" entity, which stores the reference between digest and user. It's essentially the same as the entity reference field.

I understand that this was considered:

Proposed resolution :

Create a content entity 'subscription' to store the relationship between a user and a digest.

Alternate ideas :

This method could create a large number of entities so perhaps we could make it a one to many relation. Likely one for each digest with a list of users.

What's the problem with a large number of entities? If there's a large number of relationships between digests and users, you still have to store that.

The third diagram there is supposed to be a field added to users that references the digest entities they subscribe to. I don't think it makes a difference from the database perspective, but it might be more natural depending on how people are supposed to manage their subscriptions. If the subscription is a field on the user, one could implement the management interface with a field widget, and sites might end up with an easier way to control where that UI appears. I don't know if an entity reference field can reference a configuration entity, but this could be a custom field type instead.

I think you'll want to change the entity's ID and table name, too. "subscription" seems reasonably likely to conflict with another module at some point.

DerekCresswell’s picture

FileSize
9.34 KB

May need to re install module on a test site for these features to work.

One thing I am un-sure about is enabling the field on module install. Currently the configuration will add it to the user but it remains disabled until it is manually enabled (on the manage fields page for users).
Can this be done automatically with the config? I hesitated to put in that bit since it seems like it could overwrite other fields that had been added to the user.

Currently, there is some instruction added to the Readme on how to enable the field. It can be done automatically with this snippet instead :

/**
 * Implements hook_install().
 */
function digest_install($is_syncing) {

  // This enables the subscription field on users with the correct widget.
  // The field is added with the install configuration. To avoid overwriting,
  // existing configuration, it is enabled here.
  \Drupal::entityTypeManager()->getStorage('entity_form_display')
    ->load('user.user.default')
    ->setComponent('field_digest_subscriptions', [
      'type' => 'digest_subscriptions_widget',
    ])->save();

}

Though I wanted to get others opinions before adding this. Pair this with one to automatically disable / remove the field when uninstalling.

If a formatter is wanted then it should become a separate issue.

This could use some tests. Having a little difficulty with them though. Will continue if this is ok though.

DerekCresswell’s picture

Issue summary: View changes
joshmiller’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/Readme.md
    @@ -35,7 +35,11 @@ Install as you would install any other [contributed Drupal module](https://www.d
    +After installing and enabling the module, navigate to `/admin/config/people/accounts/form-display` and enable the "Digest subscriptions" field. Make sure to set the widget of that to "Digest subscriptions" as well.\
    

    These instructions are preferrable to modifying users on install.

  2. +++ b/digest.module
    --- /dev/null
    +++ b/src/Plugin/Field/FieldWidget/SubscriptionWidget.php
    

    Why do we need a widget?

travis-bradbury’s picture

If you do need the widget, you should consider a new field type. Based on the instruction "Make sure to set the widget of that to "Digest subscriptions" as well.", using the default widget is bad. I'm assuming that using this widget on any other list_string field would be bad too.

You can disarm that footgun by making a new field type (extend the list_string field type if you want) so that your widget is the only option.

DerekCresswell’s picture

The widget just adds the descriptions to each option and in the future other details like when they send. As we speak, I am removing some of the override functions from the widget to clean it up.

The widget will still only work with the digests so perhaps an empty field type would be wise to prevent errors.

DerekCresswell’s picture

As suggested by @joshmiller here is a patch that ditches a widget in favour of using a hook to modify the the wdiget and give it description etc.

I believe this to be the not ideal solution, but am always open to options.

DerekCresswell’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
14.39 KB

We've decided to stay with the widget. I've removed some unnecessary overriding in the widget, a blank subscription field to prevent errors, and tests to ensure the page is displaying properly.

joshmiller’s picture

Status: Needs review » Reviewed & tested by the community

  • DerekCresswell committed 8abf8bf on 1.0.x
    Issue #3154614 by DerekCresswell, tbradbury, joshmiller: Add base for...
DerekCresswell’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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