distinguish between "not subscribed" and "unsubscribed"

Cool :)

Do you have a schema yet for the expanded simplenews_snid_tid table?
Also, would it be a list of events, or status?
In other words, when a user unsubscribes, do you keep the old record and add a new one, or update the record for that snid/tid combination?

How about
snid
tid
1/0 for subscribe/unsubscribe
source
timestamp

I need simplenew_roles to behave properly for a project with a fairly short deadline, so let me know how I can help -- what code needs to be written or tested :)

I agree that we don't need subscription history. At least I don't see a case yet.

As for subscription state: it saves on columns. You currently have two source and timestamp columns. If this is a state, then you're never populating both -- when you unsub, you clear the sub details, I presume?
Also, it gives a quick way to find what we need. Otherwise, subs are 'those with empty unsub timestamps' and unsubs are 'those with filled unsub timestamps' or 'those with empty sub timestamps' (the latter only if we're sure that gets cleared!)

- Subscribing via 'My account' produced 'unknown' source.

You're missing the language parameter in most of the calls to this I can see, so the source param is going to the wrong place.
I'm rearranging the parameters so source (which is always used) comes before language. That way it matches simplenews_unsubscribe_user().

- Unsubscribing via 'My account' produced 'Website' source -- I'd use all lowercase for this column

+            simplenews_subscribe_user($account->mail, $tid, FALSE, t('Website'));

It doesn't make sense to store translate stored data: things might break if site language changes?! -- better to translate it on output (if there is any UI output).

I'm attaching a new patch with the above changes.

Some subsequent things to consider:

I would define the values the core simplenews module will use in the function comment, and be quite disciplined about what values we use.
Other modules can of course send their own values -- up to them.
So far we have:
- website: something done through the website UI.
- mass subscribe: mass admin UI
- mass unsubscribe: mass admin UI
- action: Drupal actions

Shouldn't we have an 'email' source for unsubscribing?
I can't tell if simplenews_subscription_manager_form_submit() only covers users who have arrived from email.

Do we want to distinguish between an action the user takes themselves and the admin changing this? -- ie, if an admin edits another user's account. Is that possible and desirable?
I'm thinking of the following use cases:
- simplenews_roles needs to know that the user chose to remove themselves
- an admin doing a mass subscribe or user account edit should be warned that some emails are currently unsubscribed.

Awesome :)
I'll get to work on how simplenews roles can best use this.

I see this is only in HEAD -- is your plan to have this in a 2.0 release?
I'm about to make SN roles dependent on this.

No chance of it going into 1.0 then? The whole of simplenews roles and possibly other modules too in the pipeline too (see http://groups.drupal.org/eventrelated-drupal-code) will need this.

Also, I wonder whether we should rethink our data storage here.
I am changing simplenews roles to use the current system, and getting a query of "people who either are status 1 or status NULL" is a bit ugly. Seems you can't say "status != 0" without also excluding the NULLs.

Hmm yeah, this is proving really tricky to work with. Some horrible queries.
See #331080: unsubscribe reverts back after cron run
I'm afraid I think we need to rethink this.

Ah, chris, I don't think I explained properly on IRC:

status can be NULL simply if SN knows nothing about the user -- you get a NULL from the LEFT JOIN.
the actual data is always 0/1.

I've got something that seems to work, but it has to use PHP rather than only queries -- get the mails to affect with a query, then subscribe/unsub them with the SN API function.
I am concerned about its scalability.

Called by simplenews_roles_cron when required:

  /**
   * step 1: 
   * remove users who are not in the role(s)
  */
  $query_1 = "
    SELECT sns.mail 
    FROM 
      {simplenews_snid_tid} snst 
    JOIN 
      {simplenews_subscriptions} sns
      ON snst.snid = sns.snid
    JOIN 
      {users} u
      ON sns.uid = u.uid 
    LEFT JOIN /* LEFT JOIN because we want the NULLs */
      {users_roles} ur 
      ON 
        u.uid = ur.uid 
      AND 
        ur.rid IN (%s) /* $rids_string */
    WHERE 
      snst.tid = %d   /* $tid */
    AND    
      ur.uid IS NULL
    ";
      
  $mails_1 = db_result(db_query($query_1, $rids_string, $tid));
  foreach ((array)$mails_1 as $mail) {
    // Call SN API to unsub users.
    simplenews_unsubscribe_user($mail, $tid, FALSE, 'simplenews_roles');
  }
  
  /**
   * step 2: 
   * add users who are in the role(s), and not subscribed
   * but only if the prior source of unsubscription is ourselves
   * In other words, if the source of their unsub is 'website', say
   * we don't touch it.
   */
  $query_2 = "
    SELECT u.mail
    FROM
      {users} u
    JOIN
      {users_roles} ur 
      ON 
        u.uid = ur.uid 
      AND 
        ur.rid IN (%s) /* $rids_string */
    LEFT JOIN /* LEFT JOIN because we want the NULLs */
    (
      {simplenews_snid_tid} snst 
    JOIN 
      {simplenews_subscriptions} sns
      ON 
        snst.snid = sns.snid
      AND
        snst.tid = %d   /* $tid */
    )
    ON
      u.uid = sns.uid
    WHERE
      sns.snid IS NULL /* those that don't exist at all for this tid or for SN as a whole */
      OR
      snst.source = 'simplenews_roles' /* those we unsubbed ourselves previously */
  ";

  $mails_2 = db_result(db_query($query_2, $rids_string, $tid));
  foreach ((array)$mails_2 as $mail) {
    // Call SN API to sub users.
    simplenews_subscribe_user($mail, $tid, FALSE, 'simplenews_roles');
  }

The thing is, I'm not sure what data structure would be better.
I'm fairly convinced by those queries that the current one could be improved.

The more I think about things that could build around simplenews, the more I see the possibility of this kind of problem.

Example: suppose we want a module that targets customers in an online store. We could have a glue module between Simplenews and Ubercart, which sends a particular newsletter to people who have bought a particular product. But this involves the same sort of syncing as simplenews_roles, with the same complex queries and scalability.

So my feeling is that the data storage we thrashed out earlier in this issue isn't working out in actual application :(

So I'm thinking:
1. work towards something more like #536620: Subscriber API: separate subscribers from newsletter management and allow other modules to define subscribers.
2. the data storage should perhaps be two tables, one of subscriptions, one of unsubscriptions.
A hypotherical Ubercart module would cause no data to be put in subscriptions, only in unsubscriptions -- as users choose to opt out.
To get users to send to, you'd do a join with your list of users with orders and the unsubscriptions list and consider only the ones with a NULL on the right hand side of the join.
3. there's also the matter of global opt-outs, which I know is a feature on the todo list. How best should this be factored in to have sane and manageable queries? I'm thinking use unsubscriptions with a tid of 0 but I'll have to go and think about what this would mean in practice.

It looks like that other issue will cover this.

Also, this current issue is the same as #1315420: When mass subscribe exclude unsubscribed subscribers: when a user actively chooses to unsubcribe, any automatic subscription should respect that.

Sorry I've not done any work on this -- the project I worked on Simplenews Roles for finished ages ago and I've not had time to do much on it since.