As the title says! We can add a simple entity add route/action/form like most entities have.

CommentFileSizeAuthor
#15 simplenews.add-subscriber.3037036-interdiff-6-15.txt2.28 KBadamps
#15 simplenews.add-subscriber.3037036-15.patch8.95 KBadamps
#8 simplenews.add-subscriber.3037036-6.patch7.14 KBadamps
#6 simplenews.add-subscriber.3037036-interdiff-2-6.txt2.63 KBadamps
#6 simplenews.add-subscriber.3037036-6.patch7.14 KBadamps
#2 simplenews.add-subscriber.3037036-2.patch4.58 KBadamps
#2 simplenews.add-subscriber.3037036-2.patch4.58 KBadamps

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented
Status: Active » Needs work
StatusFileSize
new simplenews.add-subscriber.3037036-2.patch4.58 KB
new simplenews.add-subscriber.3037036-2.patch4.58 KB

This patch applies after #3013721: SubscriberForm overwrites existing subscription data has been committed so it won't apply now. However I'll upload it anyway to make sure it doesn't get lost.

adamps’s picture

adamps commented
berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/simplenews.links.action.yml
@@ -4,6 +4,12 @@ simplenews.newsletter_add:
 
+simplenews.subscriber_add:
+  route_name: entity.simplenews_subscriber.add_form
+  title: 'Add subscriber'
+  appears_on:
+    - view.simplenews_subscribers.page_1
+

One problem is that I think we now have 3 local actions on that page I think. We've already been discussing a bit about the existing two because the recommendation is to only have two of those. Maybe import/export should be somewhere else instead, not sure.

adamps’s picture

adamps commented

Good point. For me there are 3 already: Mass subscribe, Mass unsubscribe, Export. I have raised #3037266: Reduce number of local actions on subsribers page

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new simplenews.add-subscriber.3037036-6.patch7.14 KB
new simplenews.add-subscriber.3037036-interdiff-2-6.txt2.63 KB

Status: Needs review » Needs work

The last submitted patch, 6: simplenews.add-subscriber.3037036-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

adamps commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new simplenews.add-subscriber.3037036-6.patch7.14 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I feel like doing this needs more of a justification than just "because we can", especially due to the problem with the now 4 local actions.

What's the use case for this? Maybe mass-subscribe could better support this, for example by offering an edit link when subscribing a single e-mail?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
  1. +++ b/src/Form/SubscriberForm.php
@@ -25,7 +25,9 @@ class SubscriberForm extends SubscriptionsFormBase {
 
-    $form['#title'] = $this->t('Edit subscriber @mail', array('@mail' => $subscriber->getMail()));
+    if ($mail = $subscriber->getMail()) {
+      $form['#title'] = $this->t('Edit subscriber @mail', array('@mail' => $mail));
+    }

    Basing this on having or not having an e-mail seems a bit strange, why not use !isNew()? 

  2. +++ b/src/Form/SubscriberForm.php
@@ -77,6 +81,9 @@ class SubscriberForm extends SubscriptionsFormBase {
    */
   protected function getSubmitMessage(FormStateInterface $form_state, $op, $confirm) {
+    if ($form_state->getCompleteForm()['#is_new']) {
+      return $this->t('Subscriber %label has been added.', array('%label' => $this->entity->label()));
+    }
     return $this->t('Subscriber %label has been updated.', array('%label' => $this->entity->label()));

    This is also a bit strange IMHO, you could for example instead check the form id, which should contains the operation, a subclass or $form_state->get/set

Also, the add form IMHO looks quite weird, e.g. e-mail not being the first element.

adamps’s picture

adamps commented

Mass subscribe does not allow setting fields whereas this way does. From a usability perspective, it's not intuitive that the way to add a single subscriber is to click mass subscribe. This patch makes the subscriber behave the same way as Drupal core entities.

If you had raised your comment 3 months ago then I would have no problem dropping it. Seeing as I have now just finished writing tests then I'm not keen to throw the work away. I think we need to keep moving forwards not backwards. We already have 3 local actions and it's too many. If you feel strongly about that then you could post a patch for #3037266: Reduce number of local actions on subsribers page.

adamps’s picture

adamps commented

@Berdir Thanks for the review

1. The code as I wrote is necessary to solve #3031919: Bugs if user has blank email address.

2. Good point. Do you mean like this?

if ($form_state->getCompleteForm()['#form_id'] == 'simplenews_subscriber_add_form') {

3. This fix has not changed the form order of the email field. The code that pushes it after other fields already exists in Subscriber::baseFieldDefinitions() . However I agree it would be better to have the email earlier and I'm happy to put it to the top.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> If you had raised your comment 3 months ago then I would have no problem dropping it

Yeah, if only I'd made the same comment already 4 month ago ;) I didn't say to drop it, but maybe solve the other issue first. Just not sure how, maybe local tasks would work better, but might also be weird, especially when editing a subscriber.

2. Or just $this->getFormId(), this is the form class.

3. I know it's the same as edit, but the situation is different, for edit, the e-mail is not so important, for add, it is.

adamps’s picture

adamps commented

but maybe solve the other issue first

Sorry I don't have time to solve any extra issues. My priority at the moment is to clear all my part-finished issues so they don't need huge rerolls with upcoming big tidy up: #3055728: Convert from Simpletests to PHPUnit tests #3037140: Fix coding standards.

I guess I can commit this one with the action link code commented out and a todo linking to the other issue?

2. Good idea.

3. OK but isn't that just the way Drupal works - the add and edit forms would generally have the same order. What would you like to see?

adamps’s picture

adamps commented
StatusFileSize
new simplenews.add-subscriber.3037036-15.patch8.95 KB
new simplenews.add-subscriber.3037036-interdiff-6-15.txt2.28 KB

#3037266: Reduce number of local actions on subsribers page wasn't so hard and I have a patch ready that I will post.

2. Fixed

3. I think it is a good idea to put the email first always. When editing a user, the email is hidden, so it makes no difference. When editing a non-user it is quite common to want to edit the email - subscribers cannot edit it themselves so they often ask the admin to change it for them. In any case what we put is just a default value and anyone who doesn't like it can alter it.

I deleted core.entity_form_display.simplenews_subscriber.simplenews_subscriber.account.yml because it's no longer necessary. The access handler will automatically hide the email field.

===

I think this is a long way from being the most important issue, so hopefully we can just commit it and spend our time discussing more strategic ones.

adamps’s picture

adamps commented
Issue tags: +Plan to commit

  • AdamPS committed 8f908af on 8.x-2.x 
    Issue #3037036 by AdamPS, Berdir: Create button to add a single...
adamps’s picture

adamps commented
Status: Needs review » Fixed
Issue tags: -Plan to commit

I need to commit to unblock other issues. If you have any more concerns then please comment and I can do another commit to fix them.

Status: Fixed » Closed (fixed)

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