use array instead of hash for radio button group

[meeg]

On any of the administration pages with new/delete/modify radio buttons (e.g. AdministerForm), our DocDB installation (DocDB 8.8.6) orders the radio buttons randomly. I don't think this can be intended behavior, and AuthorAdminDisable.js expects a consistent ordering of [new, delete, modify].

I think this is a bug in the AdministerActions method of AdministerElements.pm, which exists in trunk as well as in 8.8.6. The button names are stored in a hash when an array is appropriate. This commit fixes the bug.

[ericvaandering]

Thanks. I wonder why we've never encountered this before. Maybe different versions of CGI.pm?

[meeg]

Good thought. I agree it would be hard to miss this behavior, so it must be somehow environment-dependent.

We're running on a Raspberry Pi with Raspbian Jessie, CGI.pm version 4.09-1.

I found this, which seems relevant: leejo/CGI.pm#196. I have zero Perl experience, much less CGI.pm, but it sounds like my problem pops up when you have a "recent perl version" (whatever that means) and CGI.pm < 4.26, and have not set the CGI.pm flag that forces sorting. So I guess DocDB could set that flag, though I do think using an array is the correct way to go.

[pms967]

Apart from the nuisance you've noticed, in my case this patch fixes the much more severe problem I've encountered with "Administer Event Groups" not working at all, see issue #108

#108

[meeg]

Agree - this is not purely a UI annoyance. Because of the interaction between the CGI-generated web page and the JavaScript, this breaks various forms, including the "Administer Events" page you're using and whatever I was doing 5 years ago that uses AuthorAdminDisable.js.

I should have flagged that more prominently. Thanks for confirming that I'm not alone.

[meeg]

Note that PR #98 also mentions this bug/patch.

[marcmengel]

Shouldn't line 41 here be :
unshift(@Action, "Transfer");
then, to keep Action an array?

[meeg]

Thanks, makes sense, good catch! I cherry-picked your commit back into my tree.

[pms967]

Mmmh, there seems to be a problem with the patch suggested by @marcmengel: at least on my site, it breaks the "User Administration" form (EmailAdministerForm).

If I apply that change, two things happens:

1. a new entry appears in the menu ("Transfer"; I have no idea what that means);

2. I can no longer change the group of any user. Except when selecting "Delete", the "User's Groups" list remains disabled.

Does that happen to you too? Any idea about how to fix that? I guess some other script needs to be modified to use array instead of hash (but I'm no Perl expert). For the moment, I have reverted the last patch.

[meeg]

I'm not currently running a DocDB instance (that was 6 years ago . . .) so I can't test, but -

• The original problem I was having is that all of the fooAdminDisable.js scripts are hard-coded to assume that indices 0/1/2 point to new/delete/modify. The unshift here would give you transfer/new/delete/modify, which seems consistent with your problem.
• The new patch should only add the "transfer" option if $AddTransfer is set true. EmailAdministerForm does set this parameter, and EmailAdminister does have a elsif ($Action eq "Transfer") block, so I guess "transfer" is meant to be an option on this page (nope, I don't know what it does).

So . . . my hope is that using shift instead of unshift will give correct behavior, maybe you can give that a shot?

I wonder whether the AdminDisable.js scripts need to be rewritten to be aware of the element names, and maybe also to handle the "transfer" option, but that is way over my head, my Javascript knowledge is no better than my Perl (zero!).

[pms967]

So . . . my hope is that using shift instead of unshift will give correct behavior, maybe you can give that a shot?

nope: doing so will cause an error, and the page will not work at all...

(maybe using "shift" would requires a different syntax)

[meeg]

Sorry sorry, I misread the Perl docs. push is what should work (push appends, unshift prepends, same syntax). shift is a left-pop.

[pms967]

push is what should work (push appends, unshift prepends, same syntax). shift is a left-pop.

OK, with push it seems to work fine(*). Thanks!

(*) at least, from a very quick test. I'll let you know in case some problem or other unexpected behavior will be discovered.

[meeg]

I dropped the ball here, but have now updated my fork to use push instead of unshift.