Download & Extend
Mailing List » Issues

Admin UI for individual emails + list import feature

Posted by Jaza on July 27, 2009 at 5:59am
Project:Mailing List
Version:6.x-1.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:Jaza
Status:closed (fixed)

Issue Summary

I've added quite a lot of code to this module, in order to get it to do everything I need. The two main features I've added to it are:

  1. Backend UI for each mailing list, allowing individual name / email pairs to be listed and edited.
  2. Ability to import emails into a mailing list from a CSV file

I've also moved the admin area from 'admin/settings/mailing-list' to 'admin/build/mailing-list'.

Hope others find this patch useful - and if it's worth it, might even be good to get it committed back.

Note: this patch depends on these two other patches in the queue:

#302953: Mailing List
#298791: Exporting

AttachmentSize
mailing_list_admin_ui.patch21.49 KB
Login or register to post comments

Comments

#1

Posted by litwol on July 27, 2009 at 6:41am

Hello Jaza,

Are you using this on production site ?

If you would give the two other issues you've mentioned a thorough review and set them to RTBC i would be willing to commit them soon.

Thanks, let me know.

#2

Posted by Jaza on July 29, 2009 at 12:25am

Hi litwol,

I'm currently using this module on a site that's in beta, and that's scheduled to go live next week. I've reviewed the patches in the other two issues, and they both work and look reasonably well-written to me. I've set both of them to RTBC.

#3

Posted by litwol on July 31, 2009 at 2:13pm
Status:needs review» needs work

Please reroll this patch. It failed to apply after i commited the other two mentioned patches.

localhost:mailing_list litwol$ patch -p0 < mailing_list_admin_ui.patch
patching file mailing_list.install
patching file mailing_list.module
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 106.
Hunk #5 FAILED at 198.
Hunk #6 succeeded at 307 (offset -2 lines).
Hunk #7 FAILED at 331.
4 out of 7 hunks FAILED -- saving rejects to file mailing_list.module.rej

#4

Posted by Jaza on August 4, 2009 at 9:00am

New patch, which has quite a few improvements over the old one:

  • The previous patch introduced a new 'name' field, so subscribers can enter their name as well as their e-mail. This patch keeps the new field, while also preserving the email-only behaviour that mailing_list currently has. You can now toggle display of the 'name' form field per mailing list, as a block setting. Admin listings have also been changed to show e-mail as the first column (name second, if available), to display name or e-mail in admin messages, etc. Import / export works with or without names.
  • Lots of code refactoring and cleanup, including moving most of the code to a separate file mailing_list.admin.inc.
  • More changes to the DB schema, and a full DB upgrade script from the current mailing_list schema.
AttachmentSize
mailing_list_admin_ui_1.patch 35.14 KB

#5

Posted by litwol on August 4, 2009 at 7:46pm
Status:needs work» fixed

Thank you!

I really enjoyed the new functionality adition, good job. I've commited it.

Let me know if there is something more you wish to add. I think soon would be a very good time to make an official release.

#6

Posted by Jaza on August 5, 2009 at 12:21am
Status:fixed» needs review

Thanks for committing my big patch, litwol.

Mostly looking good... except, I discovered that I made one big mistake with the DB schema changes. I made a unique key index for the 'mail' field in the 'mailing_list_emails' table. However, the unique key should be for the combination of 'mlid' and 'mail', because an e-mail should only be unique in the context of the mailing list that it's in.

I've patched the same update function, rather than writing a new update function, as I doubt that anyone has upgraded in the 6 hours since this change was committed (plus this module currently has no stable releases).

Also, a few other minor fixes:

  • Change admin block title to the form 'Mailing list: @name' (user block titles remain as just the name of the mailing list, by default)
  • Give each subscription block form a unique form name, using hook_forms()
  • Remove manual validation of 'name' field as required in subscription block form (it's got '#required' => TRUE, so it's validated automatically)
  • Add '#required' => TRUE to the 'mail' field in subscription block form
  • Apply the validate handler for the subscription block form to also work on the admin 'add / edit e-mail' form

I agree, an official release should be made soon.

AttachmentSize
mailing_list_admin_ui_fixindices.patch 4.55 KB

#7

Posted by litwol on August 5, 2009 at 12:30am
Status:needs review» fixed

@Jaza

I dont think @name field should be required. My point about disagreement is to point out that some one else may also have similar opinion. therefore i think it is a good idea to make name field optional. Each mailing list could have a checkbox that says 'require name'.

What do you think of that ?

In the meanwhile i'm commiting your patch :)

#8

Posted by Jaza on August 5, 2009 at 12:43am

@litwol

The 'name' field is not required. I agree that it's best to allow site admins to choose whether or not to have this field for each of their mailing lists. That's why my main patch in this thread added a checkbox to the configuration form for each mailing list's subscription block, called 'Show name field in subscription form'.

When this checkbox is ticked, the 'name' field is shown and is required in the subscription block form. When this checkbox is un-ticked, the 'name' field is hidden (and is obviously not required). I hope that fits with how you want it to work too. Or... are you saying that this checkbox should be in the mailing list add/edit form, rather than in the config form for a mailing list's subscription block? That sounds fine to me as well.

Anyway, thanks for committing the patch so fast!

#9

Posted by System Message on August 19, 2009 at 12:50am
Status:fixed» closed (fixed)

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

nobody click here