Collapse all groups by default. Improved 'max depth' setting

Thanks for reporting this.

I can confirm that the setting 'Collaps all groups' does currently not work.

Yes, I saw this too, and had it on my list to investigate. It used to work in D6, I'm sure, so maybe I'll compare the code.

@Jonathan: are you going to fix it? Or shall I a look at this issue?

I can look at this one, you have plenty of others to deal with. I remember there was something odd with the D6 code for this. I will go back and look.

Thanks.

I found the problem - it was a change to how the parameters are passed to theme_fieldset. In D6 they were all in a single array, but in D7 the class attributes need to be in a sub-array named #attributes. It was also wrong for 'collapsible' which was hardcoded in the call but should use the settings just like 'collapsed'. It was useful that the majority of .module can be compared between D6 and D7 so that I could see we had not changed the internal settings of those options. That made it easier to track through to find the cause.

Output from debug in _weblinks_format_group()

Collapsible and closed by default
$term->collapsible: TRUE
$term->collapsed: TRUE
theme(fieldset): <fieldset class="collapsible collapsed form-wrapper">
.
Collapsible but open by default
$term->collapsible: TRUE
$term->collapsed: FALSE
theme(fieldset): <fieldset class="collapsible  form-wrapper">
.
Not collapsible
$term->collapsible: FALSE
$term->collapsed: FALSE
theme(fieldset): <fieldset class="  form-wrapper">

Here is a patch to try. Now that we have automated testing I'd like to try to add some tests to trap this.

Great. I didn't recognize the parameter changes in theme_fieldset. The hardcoded setting 'collapsible' is then also wrong in the D6 version.

Yes, I tried to keep the organization of the module as much as possible during the migration.

I will test the patch very soon.

The patch works for me. Thanks.

Thanks foe testing. I think D6 is fine - there is no hardcoded value, atleast not in _weblinks_format_group()

I won't commit this until I have tried to write tests to trap it. Otherwise the tests would not fail anyway.

Just notice that we do very similar code in weblinks_page() when the $tid is not numeric, ie it is 'popular', 'recent', 'unpublished', 'random' from a block. That will have to be fixed in the same way. I wonder if we can re-use _weblinks_format_group() instead of having this near duplicate code? Might make it a bit clumsy, but worth looking at.

It depends. If the code will be easier to maintain then it makes sense to re-use _weblinks_format_group. Otherwise it might be better to keep it separate.

Back to testing, I am going to add the following line in the $fieldset in _weblinks_format_group()

    '#id' => 'weblinks-fieldset-' . $term->tid,

This is so that the fieldset(s) can be identified, and therefore extracted and tested for having the right attributes. Without an id I think it might be very hard to write the tests.

Here is the commit for the fieldset id line. I have added tests to check the default collapsible and non-collapsed state, then change to collpased and test again, then finally change to not collapsible.

This patch has the new tests but not the .module change, to show that some of the tests fail. The patch may look complicated, but that is just because I moved a few things around to avoid having to login as admin, then login as user, then switch again to admin.

As expected, we have test failures with the existing code. Now, here is a patch with the .test and the .module changes. In addition to the changes in #6 the testing showed me that we had been setting the 'collapsed' attribute even if 'collapsible' was turned off. This could have undesired effects for theming if using these classes. Hence I made one additional change in weblinks_get_tree() where

$new_tree[$tid]->collapsed = $collapsed || ($new_tree[$tid]->depth > $max_depth) || ($new_tree[$tid]->collapsible ? $collapse_me : FALSE);

is now

$new_tree[$tid]->collapsed = $new_tree[$tid]->collapsible && ($collapsed || $collapse_me || ($new_tree[$tid]->depth > $max_depth));

What this means is that if ->collapsible is false, then it guarantees that ->collapsed will also be false. I had actually noticed this in D6 and made a note a long time ago that it needed fixing. This line is identical in D6 so could be fixed in exactly the same way.

Thanks a lot for fixing this.

I will apply your patch and test it soon.

I just finished testing your patch and it works for me.

Thanks Gerhard. I've got the commit all ready on my local machine but I'm getting
fatal: remote error: Permission denied when accessing 'weblinks' as user 'jonathan1055'
It is odd, because nothing has changed, as far as I know, since my previous commit which I did sucessfully. I'll try again later.

It worked this time - here is the commit commit/6f05ead
Thanks Gerhard for reviewing and testing, and Pafla for bringing to our attention in the first place.

I have marked this as fixed for D7. But if you or Nancy want to correct D6 it just requires the single line change I showed in #15.

Jonathan

Regarding #15 and #20:

Before applying this fix to the D6 version I want to check the change with you. I'm not sure if your change in weblinks_get_tree is fully working as expected. Lets have a look at the current code:

    // Collapsible is more complicated than just the setting.
    $new_tree[$tid]->collapsible = $collapsible && ($new_tree[$tid]->depth < $max_depth)  && !empty($term->name);
    $collapse_me = variable_get('weblinks_collapse_'. $tid, FALSE);
    $new_tree[$tid]->collapsed = $collapsed || ($new_tree[$tid]->depth > $max_depth) || ($new_tree[$tid]->collapsible ? $collapse_me : FALSE);

'$new_tree[$tid]->collapsible' will only become TRUE when '($new_tree[$tid]->depth < $max_depth)' is TRUE. So when using your change '$new_tree[$tid]->collapsible' will become always FALSE when '($new_tree[$tid]->depth > $max_depth)' is TRUE, which in turn means that '$new_tree[$tid]->collapsed' will also become FALSE even when it may be set to TRUE.

Can you check this again?

Gerhard

Just to be clear, the code displayed in #22 is from D6, ie the old version, as it had been in D7.

The patch to the D7 code committed in #20 did not alter how $new_tree[$tid]->collapsible was defined, we only changed the setting of $new_tree[$tid]->collapsed.

You say

So when using your change '$new_tree[$tid]->collapsible' will become always FALSE when '($new_tree[$tid]->depth > $max_depth)' is TRUE.

That is true but not due to my change.

which in turn means that '$new_tree[$tid]->collapsed' will also become FALSE even when it may be set to TRUE

Yes, that is exactly the point of the change. We should not have 'collapsed' set to TRUE if the link is not collapsible.

So I think it is ok, but please do ask again if you have a specific question about what is wrong.

Jonathan
ps. I have set this to 'patch to be ported' and the version to D6, as from my dashboard I did not see that there was any update.

For ease of reference, here is the new D7 line:

$new_tree[$tid]->collapsed = $new_tree[$tid]->collapsible && ($collapsed || $collapse_me || ($new_tree[$tid]->depth > $max_depth));

I have just read the code again, and read your post again. I only ensured that ->collapsed was FALSE if ->collapsible was FALSE. I left the other logic the same and did not check the usage of $max_depth.

Are you saying that we do not need to check the depth when setting ->collapsed, because we have already used it when setting ->collapsible? Also it does smell a bit odd that we are checking ->depth < $max_depth in one place and ->depth > $max_depth - so what about when ->depth = $max_depth?

I was trying to test the $depth and discovered this:

  $max_depth = $admin ? 99999999 : variable_get('weblinks_maxfrontdepth', 1);

which makes testing the depth for real users then altering the value, and testing again somewhat tedious. Do you know why we effectively remove the depth check for weblinks admins?

Regarding #24: that's the point. At least we have to review why the check '->depth' against '$max_depth' is implemented that way.

I have no idea why the depth check is removed for Web Links admins. May be to display the full depth of groups to see how deep groups are effectively nested, e.g. for maintenance purposes? Possibly Nancy can remember?

Gerhard

If I didn't comment it, then I have no idea.

No worries. I will test it both ways, and see what makes most sense.

Hi Jonathan,

could you perform the tests you mentioned in #28 in the meanwhile?

Gerhard

OK, I have done a complete test coverage of 'max depth' behavior, on a four-level nested vocabulary, for both an admin user and ordinary user. I used the global settings 'Make groups collapsible' = Yes and 'Collapse all groups' = No. Each separate group also had 'Collapse by default' = No.

Summary of problems:

1. For ordinary users one extra level is shown compared to the setting. 1 shows two levels, 2 shows three levels. This is because internally the depth starts at 0.
2. For admin users the links above the threshold should be collapsed but they are not. This is because the $max_depth is set at 99999999
3. There is no clear setting for 'show all groups' on the admin form
4. For ordinary users we get differing behavior between max_depth = blank and not set. ie the default behavior is not consistent with the description in the admin menu

My suggestions are for 1 we can fix it by changing the test from > $max_depth to >= $max_depth.
2 can be fixed by removing the code which sets the 99999999 value and altering the 'collapsible' setting
For 3 we should either implement 0 = unlimited, or have a selection drop-down with 'unlimited' as the default option.
We can fix 4 by not allowing a blank value to be saved in the variable. This can be done via the one of the options in 3.

Here is a patch which addresses all of the problems above

1. Fixed in two places, line 1184
   if (!$admin && ($term->depth >= $max_depth || ($ismainlinkspage && !$show_this_term)))
   and line 1225
   $new_tree[$tid]->collapsed = $new_tree[$tid]->collapsible && ($collapsed || $collapse_me || $term->depth >= $max_depth);
2. Fixed. Line 1126 $max_depth is a straight get of the variable. Defaults to 100 if not set, which simplies the logical checks later.
   $max_depth = variable_get('weblinks_maxfrontdepth', 100);
3. Fixed by changing the plain text box to a selection drop-down with 'unlimited' as the default option, plus values 1 to 20. The numeric value behind 'unlimited' is 100, to simplify the logic tests
4. Fixed curtesy of the new selection drop-down

Many thanks for your exhaustive and detailed tests.

I agree with all your changes. But why shouldn't there an exception for the admin user to have always all groups expanded (not collapsed). I thinks that was the initial intention of the special limit 99999999. Or to make it clearer we can define an extra setting like 'Disregard maximum group depth when admin'. What do you think?

I will test the patch tomorrow.

The special limit of 99999999 is a cumbersome way to attempt this, and even if the intention was to have all links always expanded for Admins it could be done using proper logic. Anyway, I disagree that Admins would always want to see all links expanded. If the site content is such where they want to limit the number of links for ordinary users then there should be some visual indication and reminder of this when the Admin views the page. Collapsing the deeper groups which are going to be hidden from the ordinary users I think was the original intention, judging by the code as-is. At least, this is a good idea anyway and I think we can finally implement it now. Also, if the admin really does want to see all links expanded they can temporarily switch off the global 'Make groups collapsible ' option.

Another idea would be to have two 'max depth' settings, one for the main page and one for all the other group pages. I can imagine scenarios where only the level 1 links should be shown on the main page, but unlimited depth of links should be shown when viewing any other term/group. That would be easy to implement, gives more flexibility, and might ease your concern about having the deeper links collapsed for Admins on the front page. How about it?

Actually, now I've had second thoughts, and your idea of giving Admins the choice is better. I'll add a three-way option for what to do with links lower than the max depth: (1) view the group collapsed (2) view the group expanded and (3) hide the group just like for ordinary users.

You are welcome to test the patch in 31, as most of that will remain and it will be valid testing.

Good idea.

OK, I will test the patch later.

It was very straight forward to add the secondary option for 'maximum depth (group pages)' and to give Admins a three-way override. Here's an updated patch covering all in 31 plus the new options.

Tested and works great.

But there are some spelling issues:

+++ b/weblinks.admin.inc
@@ -157,19 +157,35 @@ function weblinks_admin_settings() {
+    '#description' => t('This controls how many group levels will be displayed on the main Weblinks page.'),

The #description should end in '... on the main Web Links page.'.

+  $form['page']['weblinks_admin_override'] = array(
+    '#type' => 'radios',
+    '#title' => t('Override behviour for Weblink Administrators'),
+    '#options' => array(
+      'expand' => t('Expanded fieldset'),
+      'collapse' => t('Collapsed fieldset'),
+      'hide' => t('Hidden (as for ordinary users)'),
+    ),

The #title should be 'Override behaviour for Web Links Administrators'.

And instead of 'fieldset' we shoud use 'group' (in expand and collapse).

Thanks for testing and for spotting my spelling mistakes. I also changed 'behaviour' to 'behavior' as I understand that the Drupal standard is for US English spelling not UK spellings.