Permissions problem

I'm working on a patch to fix this, but I need some clarification first: what is the meaning of the "subscribe to content" permission? I would think this should control whether nodes generally have Subscribe post links or not (resulting in comment/thread subscriptions); "subscribe to taxonomy terms", "content types" and "blogs" would then control whether users can set criteria to be notified about newly created nodes (and their comments?) that match the criteria.

Unfortunately, the code doesn't seem to agree, but I have a hard time understanding what it's trying to tell me...

Thank you for the clarification. Things are a bit more complicated than just switching 'path' => 'subscriptions' over to "maintain own subscriptions", because /subscriptions/node is the default and it's (rightfully) inaccessible without the "subscribe to content" permission, but I can handle that, once I know how to proceed.

What about the comments to, say, taxonomy terms? If a node is sent due to a taxonomy subscription, should comments to that node also be sent, even though the user does not have the "subscribe to content" permission?

I would think yes. I'm particularly thinking about forum subscriptions. I intend to allow subscribing to forums, but not to "content", i.e. I don't want to have Subscribe post links on any nodes. When a user subscribes to a forum, they should receive topic nodes as well as comments, without having to manually subscribe to each topic in addition to the forum subscription.

I think this should apply to all subscription types (taxonomy terms, content type, and blog), and "subscribe to content" should only control whether we allow fine-grained subscriptions on a per-node basis, i.e. whether there are Subscribe post links on all nodes or not, and nothing else. Do you agree?

probably your permission fixes should not change the current behavior for comments..

You're right, I got ahead of myself. This patch fixes the permissions problems — please take a look.

What have I changed:

1. Created a new default form at /subscriptions and /user/NNN/subscriptions, accessible with the tag "maintain"; this page comes up if the user has nothing but maintain permission, and it says "You have no subscription permissions enabled!".
2. Changed the user_access() calls for the /user/NNN/subscriptions pages to pass the $account: either we're the user himself (then it doesn't make a difference), or we're an administrator who got there through our admin users subscriptions permission, and in the latter case it's the user's subscription permissions, not ours, that should determine the accessibility of the subforms.
3. Changed the code for the submenus to check the corresponding permission, and take the first one that is allowed as the default subform and MENU_DEFAULT_LOCAL_TASK.

   The order of processing determines which one will be selected if there are more than one. "threads" has a weight of -1, so it always comes first, and if it's enabled, that's fine, but the order of blog/type/taxonomy is determined by the alphabetical order of the translations, so the default might not be the first one. Maybe it would make sense to force a given order — after all, "threads" always comes first and "rss" always last, and it's arguable whether the ones in between really need to be in alphabetical order...

4. Swapped the comment and blogs /user/NNN/subscriptions page code sections so that comment takes precedence.

There already was some repetition of code for the different subscription types, and with the code I added, it has gotten worse, but I chose to leave it like that for now, so that you can look at the diff and see what I've done. I'll be glad to clean it up if you want.

Is there anything I can do to get this rolling?

It looks like this patch was done against 1.62 instead of 1.68 which is latest.

Ouch! I hadn't realized that you had done an update. I hate to lose the work you've done for 1.68, and we desperately need to have a stable version to build on. 1.68 would seem to be the better choice.

If you want, I'll look into recreating the patch against 1.68, so you can apply it against the latest. Which way do you want to go?

Thank you for coming back!

Hi arthurf,

I want to do some serious cleaning up of this module over the summer so that it is more useable for folks and that more people can work on the code base.

Great news! I'll leave for a week in a week, but other than that I intend to work on Subscriptions as time permits.

I'm confused -- in HEAD/1.69 you wrote:

NOTE This was rolled back to 1.62 to apply patches and start getting code released. Some updates have been lost

but 1.62 was 7 months ago (branch point for DRUPAL-5), and there are lots of differences, both to the prior 1.68 as well as to 1.62. In #11 you write "stick with the 1.63 and move from there", but 1.63 is 6.5 months old, too.

My patch was based on 1.62.2.45 2007/04/18, which was (and still is!) the current tip of the DRUPAL-5 branch (available for download as 5.x-1.x-dev, as indicated in the Version field of this issue). Isn't that the branch that we should be working on?

I think the new HEAD/1.69, Sat Jun 30 20:02:11 2007 is the result of applying my patch to 1.62.2.38, Fri Mar 9 21:19:49 (tagged DRUPAL-5--1-9, available for download as 5.x-1.9), so you'd be discarding 1.62.2.39 through 1.62.2.45 (mindless' commits since 2007-03-09) as well as 1.63 through 1.68 (your commits in MAIN since 2006-11-26).

I'm struggling to understand the CVS versioning/branching, and maybe I have it all wrong... You probably have to decide between MAIN and DRUPAL-5, but are you sure you want to cut both branches short. Also, will you be able to put a DRUPAL-5--1-10 tag onto MAIN rather than into the DRUPAL-5 branch?

The issues in the queue seem to be about evenly distributed between 5.x-1.9 and 5.x-1.x-dev, so I'm not sure which one is the better base. Please double-check and let us know from which version you want to proceed.

Thanks!
Hans

Hi arthurf,

I'm not sure I fully understand this CVS stuff, but I'm attaching a screenshot of the CVS History view in Eclipse, and will try to explain my interpretation:

I think you've applied the patch in #5 to 1.62.2.38 and committed it as 1.69 to the MAIN branch. However, if you want the patch to ever show up as a release for 5.x, it needs to go into the DRUPAL-5 branch as well (the MAIN branch will evolve towards D6 and eventually spawn off a DRUPAL-6 branch).

So, in order to pursue development for D5, we need a version 1.62.2.46. You have two options to choose from: either

• copy 1.69 back into the DRUPAL-5 branch — this would discard the work done by mindless since 5.x-1.9, or
• check out 1.62.2.45 and apply the patch to that version (it will apply because it's really based on that version), and commit it.

Either way you'll lose the work you did in the MAIN branch (1.63 through 1.68), which is unfortunate, but apparently you've already realized that.

We'll hopefully be able to clean up a few more issues in the DRUPAL-5 branch, and when the time comes for migrating to D6 (which is drawing close already!), you'll have to copy the tip of the DRUPAL-5 branch to MAIN and take it from there. Any remaining issues will need to be fixed in both branches, so the more we can do now, the less duplication of effort we'll have then...

I'd go for the second option, because mindless has done a lot of good work since 5.x-1.9.

Here's the patch for 2.x (1.62.2.45.2.2).

See #5 for explanations.

One of the subitems must be the default item, the one that is pre-selected with /user/N/subscriptions or /subscriptions. It used to be 'threads' (aka 'node' aka 'comments' aka 'content') which had

'type' => MENU_DEFAULT_LOCAL_TASK, 'weight' => -1,

but we don't know whether 'threads' will be accessible, nor any of the others.

If the user has maintain own subscriptions, then in both User subscription pages and My subscriptions pages there will always be a 'maintain' item, which says "You have no subscription permissions enabled!". The index of these items is $dft_index.

[I know that it's 1 for the User subscription pages, but if I have to duplicate code, then I prefer to duplicate it as closely as possible.]

Since we don't know which subitems are accessible, we must try them one by one, and for the first one that is actually visible (whose $access is true) ++$dft_set becomes 1 and it gets 'type' => MENU_DEFAULT_LOCAL_TASK assigned. Also, its command verb is patched into the item at $dft_index to overwrite 'maintain', which is not needed, if we have at least one subitem.

$dft_set is both a flag (as long as it's 0, we have no default subitem set yet) as well as a counter to ensure that we catch the first and only the first accessible subitem, and this first subitem becomes the actual default subitem.

Does this make sense? I'm not sure just how much comment you'd like to have. Should I insert this text into subscriptions_menu()?

You've done a lot of work on my original patch, which was only a starting point with lots of good opportunities for refactoring. I've been away for a few days and I haven't gone through each stage you posted, but instead I've just installed the current 2.x-dev version, which also has a lot of other changes.

Now I find that you've apparently removed the /user/NNN/subscriptions menu completely — this is a pity, because it allowed the administrator (or any user with admin users subscriptions privileges) to look at and change other users' subscriptions. Without /user/NNN/subscriptions we're again completely in the dark as to what our users are doing, and we have to resort to SQL editing to fix problems.

This renders the admin users subscriptions privilege and the Show Subscriptions users menu under "My account" option useless, and it's a HUGE step backwards. Please reconsider this.

Factoring out the two menus is a bit of a pain, but dropping one is not the solution, at least not that one!

(BTW, I found that a recent comment was mailed out to only one user even though I know there are many more subscribed, but since I can't look at the other users' subscriptions anymore, it's difficult to find out what makes her different. I'll open a new issue when I find out...)

Ah, yes, it's still there, I didn't understand exactly what was wrong at first. Let me try again.

Here's how it used to be (and how it should be IMHO), using user 1, to keep things simple:

1. In the Navigation menu there is My account aka /user/1 and My subscriptions aka /subscriptions.
2. On the My account page (title is "Admin") there's a local menu with View aka /user/1, Edit aka /user/1/edit, and Subscriptions aka /user/1/subscriptions.
3. As administrator I can use /user/5 to look at another user (say, Peter, and the title is "Peter"), and I get the same local menu with View aka /user/5, Edit aka /user/5/edit, and Subscriptions aka /user/5/subscriptions.

Here's what I see now (CVS 1.62.2.45.2.7), changes in bold:

1. In the Navigation menu there is My account aka /user/1 and My subscriptions aka /user/1/subscriptions, and the latter is a subitem of the former.
2. On the My account page (title is "Admin") there's a local menu with View aka /user/1 and Edit aka /user/1/edit, but NOT Subscriptions aka /user/1/subscriptions.
3. As administrator I can use /user/5 to look at Peter (title is "Peter"), and I get the same local menu with View aka /user/5 and Edit aka /user/5/edit, but NOT Subscriptions aka /user/5/subscriptions! (That's what I meant in #22.) Instead,
   • the My subscriptions item in the Navigation menu now links to /user/5/subscriptions!
   • The title of /user/5/subscriptions is "My subscriptions", even though it correctly shows Peter's subscriptions (this is an old bug, it should be "Peter's subscriptions").
   • /subscriptions is still available, but I see no menu item to get there.
   • Also, the title of the /subscriptions page is "my subscriptions" — it should be "My subscriptions".

I would expect My subscriptions to always go to my subscriptions, even when I'm looking at another user, and /user/NNN/subscriptions should appear as a local item on the /user/NNN page.

I just discovered that there can be a secondary local menu below View/Edit/Subscriptions! It appears on /user/NNN/edit if you have defined custom fields (with categories) under /admin/user/profile. For each category there is a secondary menu item, leading to /user/NNN/edit/<category>.

It would be great if Subscriptions would follow this model: keep the primary local menu, use "<username>" as the page title, and display Threads/Categories/Content types/RSS Feed (e.g. /user/NNN/subscriptions/taxa) as secondary local menu.

Never mind #25, I see now that you've already done that.

Digging deeper, I found that I didn't see the /subscriptions menu item because I had disabled it. So I have to update #24 above — the first item in the second list should read:

1. In the Navigation menu there is My account aka /user/1 and My subscriptions aka /subscriptions, and if I select My account, then there appears a second My subscriptions menu item (as subitem of My account) that links to /user/1/subscriptions, and there is no Subscriptions local menu item.

Your strategy to avoid duplicating the code for building the menu is certainly good, but this

        $items[] = array(
          'path' => $base,
          'title' => t('My subscriptions'),
          'access' => TRUE,
          'callback' => 'subscriptions_page',
          'callback arguments' => array($uid, $stype),
        );

creates My subscriptions navigation menu items in both cases, instead of creating one of those and a local Subscriptions menu item. I don't understand your logic completely, but conditionally inserting a local menu item solves the problem:

        if (isset($uid)) {
          $items[] = array(
            'path' => $base,
            'title' => t('Subscriptions'),
            'access' => TRUE,
            'type' => MENU_LOCAL_TASK,
            'callback' => 'subscriptions_page',
            'callback arguments' => array($uid, $stype),
          );
        }
        $items[] = array(
          'path' => $base,
          'title' => t('My subscriptions'),
          'access' => TRUE,
          'callback' => 'subscriptions_page',
          'callback arguments' => array($uid, $stype),
        );

At first I put the second part into an else clause, but then the My subscriptions menu item disappeared completely (after visiting /admin/build/menu to flush the menu cache). I don't understand why it behaves that way, but doing the second part in either case fixes the problem.

The analysis so far was based on subscriptions.module 1.62.2.45.2.7. In the meantime you're at 1.62.2.45.2.10 (I love the new categories display!), but the menu item mix-up is still the same. In addition, /subscriptions now displays "Page not found". I won't try to fix this, because My subscriptions is useless IMO and I'll hide it anyway. The attached patch applies to the current 1.62.2.45.2.10.

I've also added

      if (arg(0) == 'user') {
        drupal_set_title(check_plain($account->name));
      }

at the end of subscriptions_page() to display the correct title for the /user/NNN/subscriptions pages.

Argh, never mind that bit about /subscriptions now displaying "Page not found".

After refreshing the menu cache it works fine now. Sorry about that...

Re-rolled for 1.62.2.45.2.11.

Re-rolled for 1.62.2.45.2.12. Also fixed two typos.

Thanks