Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Now are able to show unconfirmed and unsubscribed subscribers as well, and filter on them, but the subscription status is not visible.
Proposed resolution
Add " (Unconfirmed)" and " (Unsubscribed)" behind the subscription, this needs a custom field formatter that subclasses entity reference label.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff-show_subscription-2420129-31-33.txt | 853 bytes | edurenye |
#33 | show_subscription-2420129-33.patch | 4.85 KB | edurenye |
#31 | show_subscription-2420129-31.patch | 4.89 KB | edurenye |
#31 | interdiff-show_subscription-2420129-27-31.txt | 898 bytes | edurenye |
#27 | interdiff-show_subscription-2420129-24-27.txt | 2.82 KB | edurenye |
Comments
Comment #1
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedAdded the new formatter and edited the config file.
Comment #3
BerdirThis hasn't been updated.
simplenes_subscription_status is a better name for this.
This should also mention subscription.
Visible strings must always use t() or $this->t() if available so that they can be translated.
Also not sure this here makes sense. I'd just leave it empty in this case. You don't really need the if/else then, you can always loop over items.
This still needs to show the actual subscription label like the parent implementation does.
Comment #4
BerdirComment #5
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, all changed, now the label is like the parent.
Comment #6
miro_dietikerThis is a UI issue, please provide a screenshot.
Comment #7
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThis still requires
t()
function...Comment #8
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedSorry, I did a rollback and I forgot to add the t() again.
Screnshot:
Comment #9
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedThis also needs some tests as said by Berdir (comment #4).
Comment #10
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedDo we want to have status linkable?
We already have link on the newsletter, and status links points to the same page.
Btw, how can I add a subscriber with Unsubscribed/Unconfirmed status?
Comment #11
miro_dietikerDiscussed. The subscription status should not get a separate column. It needs to be added to the subscription (newsletter) label and each subscription should be output as an item of a subscription list.
Comment #12
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI did it as we discussed and also add the tests.
Screnshot:
Comment #13
miro_dietikerLooks pretty OK for me. One nit.
No variable assignment here. Just check for it on one line.
Comment #14
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, now it's in one line.
Comment #15
LKS90 CreditAttribution: LKS90 commentedLooks good now!
Comment #16
miro_dietikerremove one "status".
Remove "Formatter"
Can be done on commit since all nonfunctional.
Comment #17
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone.
Comment #18
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedSorry, this was the interdiff.
Comment #20
BerdirHm. While this is nice to make it show up after it... there is a problem with it.
This isn't going to work for left-to right languages, as the output won't be in the right order for them.
I think we need to change this to use t('@label (Unsubscribed)') and same for the other variation, so it can be translated properly. And then build the full output ourself and don't rely on the parent.
Comment #21
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, but rebuild totally the method could rely on repeated code, I think you mean just for the title.
Comment #22
edurenye CreditAttribution: edurenye commentedComment #23
BerdirNo, you must have two separate t() calls for the two cases with additional label to have it properly translatable. for the confirmed case, you can just use the entity label as is.
Comment #24
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, I made those changes.
Comment #25
BerdirHm.
Is it really helpful to have the newsletter there linked? it doesn't seem like a common operation to got to the newsletter form from there, and having all those links on a page with lots of newsletters seems quite distracting.
Also, when you edit the view and disable the link option then this doesn't work anymore because the parent structure doesn't match anymore.
Let's change this to extend from EntityReferenceFormatterBase, no longer call the parent at all and just set #markup => $output.
Formatter that displays a newsletter subscription with the status.
You can just directly assign $label.
And the comment should be something like "// Do not explicitly display the status for confirmed subscriptions."
This will then become #markup.
Comment #26
BerdirWe have a pretty good demo module now to test this with some data. We should make sure to have at least one unconfirmed and unsubscribed subscription set up in simplenews_demo_install().
Comment #27
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedOk, just unconfirmed was missing in demo, now is added.
Comment #28
BerdirThat's how this API works. There are arguments that except the constant, it just expects NULL, TRUE or FALSE. And what you want is TRUE.
Comment #30
BerdirTest fails can be ignored, that's just missing dependencies. You might want to run the demo test locally though to make sure that is passing.
Comment #31
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone. In my machine it pass the tests.
Comment #32
BerdirI'm sorry but this is still not correct.
There is no argument that would accept that constant. It works, but that's only because the value is cast to a boolean.
Comment #33
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone.
Comment #35
juanse254 CreditAttribution: juanse254 at MD Systems GmbH commentedSame comment as above, delete the comment or give a more accurate description.
Besides that looks good to me, test failing due to core issues.
Comment #36
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedThose comments are not saying the same.
Yes, it's failing for a core change, there is a issue open for this, so it's not related to this issue.
Comment #37
juanse254 CreditAttribution: juanse254 at MD Systems GmbH commentedRight, by the way the related issue is #2540586: Link "Add newsletter" not rendered properly as soon as that gets fixed this will be green as well.
Comment #38
BerdirNow it looks fine, committed.