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

ACP Component #31

Merged
merged 33 commits into from
Feb 6, 2015
Merged

ACP Component #31

merged 33 commits into from
Feb 6, 2015

Conversation

iMattPro
Copy link
Contributor

@iMattPro iMattPro commented Feb 2, 2015

No description provided.

@iMattPro iMattPro added this to the 1.0.0 Beta milestone Feb 2, 2015
@iMattPro iMattPro mentioned this pull request Feb 2, 2015
@iMattPro iMattPro mentioned this pull request Feb 2, 2015
10 tasks
@michaelcullum
Copy link
Member

@rxu @ForumHulp @Pico Please could you all review this (and post back if you think it looks okay or make any necessary comments)?

I shouldn't be the only one reviewing @VSEphpbb's stuff, you guys are more than capable of finding errors/commenting as well as you're all good developers. Don't be afraid to question stuff. Even small things like too many line breaks, spacing, typos etc.

@rxu
Copy link
Contributor

rxu commented Feb 4, 2015

When you install this branch to the board and try to create a group rule, you get There are no user groups. at the Group option. When you try to submit the form, you get The submitted form was invalid. Try submitting again. error as 'autogroups_group_id' value is 0.
I guess this is because there're no autogroups at the very start, but you have no option to create even 1 currently.
Unless I'm missing something obvious here.

@iMattPro
Copy link
Contributor Author

iMattPro commented Feb 4, 2015

@rxu You need to create some Groups. Auto Grouping only works for custom user groups as it would not make sense to be auto-grouping people in and out of the default groups that come with phpBB, as those include registered users, admins, mods, etc. This is meant for "User defined" groups

@rxu
Copy link
Contributor

rxu commented Feb 4, 2015

@VSEphpbb This should be stated, f.e., at the ACP module description, as it could be unobvious. Also, if there're no custom groups exist, it looks senseless to be able to submit the form with no user group selected as all you'll get is the error.

@iMattPro
Copy link
Contributor Author

iMattPro commented Feb 4, 2015

Fair point. I think it's better to show the user the error of their ways in usage. So instead if there are no valid predefined groups to use, the menu will show No groups available, and if the user submits, they get a full explanation of what they need to do.

@rxu
Copy link
Contributor

rxu commented Feb 4, 2015

It's much better now, thanks. Although, for my taste, I'd add the explaination to the ext ACP front page rather than making user go the whole way to the error message :P
Anyway, I think functional issues like this can be resolved later on. The PR looks good for me.

michaelcullum added a commit that referenced this pull request Feb 6, 2015
@michaelcullum michaelcullum merged commit d1c21d3 into phpbb-extensions:master Feb 6, 2015
@iMattPro iMattPro deleted the acp branch February 6, 2015 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants