Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed the "simple" permissions management page #623

Merged
merged 1 commit into from Jul 19, 2013

Conversation

emanuele45
Copy link
Contributor

I seem to remember someone said it was useless.

This branch depends on #622
I kept this one on its own branch be cause I was not sure if everybody agree on it. ;)
Obviously comments are welcome.

@StealthWombat
Copy link
Contributor

I'm inclined to think it's useless. Permissions are, by nature, complex.

One thing that does bug me about the current setup is having "permissions" on one page and "post moderation" on another. The permissions page already includes the relevant options.

The "post moderation" page just confuses the issue, IMO, and is likely to cause problems for users, rather than prevent them. That's not counting the extra code required to run it (teh bloatness) and the fact that the presentation is not exactly easy to read (columns tend to run together and need more definition).

Suggestion: kill "post moderation" as a separate category, and just assign permissions, as you want them, on one page. Less code. Less confusion. Gotta be good.

@StealthWombat
Copy link
Contributor

Just looking a bit deeper. I think the whole system needs a bit of a re-think. Take the "Modify group" option. That leads to two different pages, depending on where you start. You either go to area=permissions;sa=modify;group=-1;pid=1 or you go to area=permissions;sa=modify;group=-1;view=classic

These pages are supposedly both for editing permissions for the same group, but have dfferent layouts. This is a dog's breakfast, because in both cases you have to edit a profile, which then gets applied to a board. So, why are there two different pages for accomplishing what is, effectively, one task? It's as clear as mud.

Surely the one page headed "Permissions for group "Guests" in profile "Default"" (area=permissions;sa=modify;group=-1;pid=1) should be adequate, if properly set up.

I'm quite sure that it should be possible to streamline this, so that there are fewer pages to cross reference, while still allowing the same degree of control. Honestly, it's little wonder that so many people don't like 2.0.x much. It's not very user-friendly.

@eurich
Copy link
Member

eurich commented Jul 8, 2013

Grea to see this ..Never liked the "simple view", never ever used it personally..

Signed-off-by: emanuele <emanuele45@gmail.com>
@emanuele45
Copy link
Contributor Author

Just looking a bit deeper. I think the whole system needs a bit of a re-think. Take the "Modify group" option. That leads to two different pages, depending on where you start. You either go to area=permissions;sa=modify;group=-1;pid=1 or you go to area=permissions;sa=modify;group=-1;view=classic

I think you are doing something weird as usual... :P
Yes, in theory depending on where you click you may have different pages, but the second link you posted is the one that you get when you are actually switching the view, so something you are doing deliberately, otherwise it is consistent and is uses simple or classic depending on what you select. ;)

BTW: simple is gone with the PR, so not a big problem. 😇

In the meantime I rebased so it merges, if no objections I'll go on.

@norv
Copy link
Contributor

norv commented Jul 14, 2013

Thanks, Ema. I think it's better without it. Then we might want to take yet another look at the permissions view (and maybe take the plunge too at it), 'cause this stuff ain't very cool. I still don't have a clear image of the changes beneficial to it, at most bits (of wisdom, of course 👼), but there must be something we can do!

@emanuele45
Copy link
Contributor Author

Yep, true.
Something really easier to deal with.

I think there is already a discussion going on at elkarte.net...don't remember exactly where. :P

// For guests, just reset the array.
if (!isset($context['group']['id']) || !($context['group']['id'] == -1 && $any_group))
// Guests can have only any, registered users both
if (!isset($context['group']['id']) || !($context['group']['id'] == -1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intention of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the comment says. :P (I hope 👼 )

Guests can have only the "any" permissions and since all the permissions are "any" by default unless otherwise, this checks that the group is set and is not -1 to add the "own".
Yes, the if is a bit screwed, may be:

if (!isset($context['group']['id']) || $context['group']['id'] != -1)

$any_group doesn't exist any more because it was true only for some permissions in the 'simple' view (see https://github.com/elkarte/Elkarte/pull/623/files#L1L622)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there is an else, then maybe:

if (isset($context['group']['id']) && $context['group']['id'] == -1)
$bothGroups['any'] = $own_group;
else
$bothGroups['own'] = $own_group;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, thank you. That's logically equivalent and easier to read. No biggie though. Lets see how it works and maybe indeed we'll simplify the expression.

norv added a commit that referenced this pull request Jul 19, 2013
Removed the "simple" permissions management page
@norv norv merged commit e63fa46 into elkarte:master Jul 19, 2013
@emanuele45 emanuele45 deleted the remove_simple branch July 19, 2013 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants