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

Allowing no default group on register #1202

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

avsdev-cw
Copy link
Contributor

With this patch, setting the default group to "null" in the configuration means that newly registered users default to not being in a group. Previously this caused a "Registration is broken" error.

Incidentally, the translation for "ACCOUNT.REGISTRATION_BROKEN" does not exist either...

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #1202 (80a3e16) into master (5494cd9) will decrease coverage by 2.34%.
The diff coverage is 60.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1202      +/-   ##
============================================
- Coverage     70.68%   68.34%   -2.35%     
- Complexity     1985     1986       +1     
============================================
  Files           173      169       -4     
  Lines          6905     6643     -262     
============================================
- Hits           4881     4540     -341     
- Misses         2024     2103      +79     
Impacted Files Coverage Δ
app/sprinkles/account/src/Account/Registration.php 74.11% <50.00%> (-1.47%) ⬇️
.../sprinkles/admin/src/Controller/UserController.php 99.11% <100.00%> (-0.32%) ⬇️
...rinkles/account/src/Database/Models/Permission.php 60.00% <0.00%> (-7.86%) ⬇️
app/sprinkles/admin/src/Sprunje/UserSprunje.php 16.66% <0.00%> (-3.75%) ⬇️
...pp/sprinkles/account/src/Twig/AccountExtension.php 76.47% <0.00%> (-3.53%) ⬇️
app/sprinkles/core/src/Csrf/SlimCsrfProvider.php 26.92% <0.00%> (-2.71%) ⬇️
app/sprinkles/account/src/Database/Models/User.php 75.00% <0.00%> (-2.40%) ⬇️
...rinkles/account/src/Repository/TokenRepository.php 62.06% <0.00%> (-1.87%) ⬇️
.../account/src/ServicesProvider/ServicesProvider.php 81.10% <0.00%> (-1.30%) ⬇️
app/sprinkles/core/src/Router.php 72.00% <0.00%> (-1.08%) ⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8438e...80a3e16. Read the comment docs.

@lcharette
Copy link
Member

Sounds good. I'm just curious about this line :

The config would need to be explicitly false? Testing for false, null and/or '' might be better?

Note in long term, I'm looking into setting the default group in the db or something completely different.

@lcharette lcharette changed the base branch from master to hotfix March 3, 2022 01:22
@avsdev-cw
Copy link
Contributor Author

That already checks for false & null (null evaluates to false). the only case it doesn't check is '' which could be added?

@avsdev-cw
Copy link
Contributor Author

@lcharette Any movement on this?

Thanks

@lcharette lcharette added this to the 4.6.5 milestone Mar 15, 2022
@lcharette lcharette merged commit 260d5d3 into userfrosting:hotfix Mar 15, 2022
@lcharette
Copy link
Member

I merged it. This might change in V5 anyway.

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.

2 participants