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

Allow null group assignment for users (Issue #867) #964

Merged
merged 22 commits into from Jul 12, 2019

Conversation

@amosfolz
Copy link
Contributor

commented Apr 11, 2019

I think this is ready for a review.
Maybe someone else can suggest a better way to handle showing an empty option using select2.

Ref.: #867

amosfolz added some commits Apr 9, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Looks like something is broken with the new migration : https://travis-ci.org/userfrosting/UserFrosting/jobs/518564703#L873

amosfolz added some commits Apr 11, 2019

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

I am going to try to see if I can figure out what is causing Travis to fail here. I noticed the migrations are all completing without any errors...
image

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

The good news is, you're not crazy. I checked out your branch and can't replicate the Travis issue. Even if I replicate Travis env locally (ie not using the :memory:database), the only issue I see is related to testGetAsset.

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

One thing to note, Travis complains about UpdateUsersTable.php:56, which means the issue would be with the down method...

Actually, it could be a MySQL config thing. The error is different on PgSQL. Something might be different between Travis MySQL setup and local/Homestead one.

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Okay, so I believe the PgSQL error is actually related to this https://github.com/userfrosting/assets/releases/tag/5.0.4

amosfolz added some commits Apr 14, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Okay, so I believe the PgSQL error is actually related to this https://github.com/userfrosting/assets/releases/tag/5.0.4

And I believe it was fixed in hotfix here : 36ed889

Since this PR target UF 4.3, I suggest we leave it as is for now and look at it once we have released 4.2.1. We'll need to merge hotfix into develop at some point anyway.

@lcharette lcharette added this to the 4.3.0 milestone Apr 14, 2019

@lcharette lcharette changed the title Issue #867 Allow null group assignment for users (Issue #867) Apr 14, 2019

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Is this something in the migration that needs to be fixed or is there possibly something in the test environment/configuration causing this?

@codecov

This comment has been minimized.

Copy link

commented Jun 15, 2019

Codecov Report

Merging #964 into develop will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #964      +/-   ##
=============================================
+ Coverage      66.93%   66.96%   +0.02%     
- Complexity      1913     1918       +5     
=============================================
  Files            159      160       +1     
  Lines           6684     6693       +9     
=============================================
+ Hits            4474     4482       +8     
- Misses          2210     2211       +1
Impacted Files Coverage Δ Complexity Δ
.../src/Database/Migrations/v430/UpdateUsersTable.php 100% <100%> (ø) 3 <3> (?)
.../sprinkles/admin/src/Controller/UserController.php 99.79% <50%> (-0.21%) 110 <0> (+2)

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 df8495e...71b2cd1. Read the comment docs.

amosfolz added some commits Jun 15, 2019

@lcharette lcharette self-assigned this Jun 21, 2019

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I think this will need to be a non-reversible change. I have been trying for hours to get a down migration to work that will set the column back to not null and have had no success.

And now that I think about it, because this PR makes changes to templates and asset, without a way to reverse the changes in those files other than reverting back to a prior version of UF, there is really no reason to reverse this migration as it will break the interface by having an empty option in UI.

image

@lcharette

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

I think this will need to be a non-reversible change. I have been trying for hours to get a down migration to work that will set the column back to not null and have had no success.

If the change in the db is not reversed, and the files are reversed to 4.2.x, does it still works? Obviously, you won't be able to select "no group", but as long as the db still works, we should be good.

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

I think this will need to be a non-reversible change. I have been trying for hours to get a down migration to work that will set the column back to not null and have had no success.

If the change in the db is not reversed, and the files are reversed to 4.2.x, does it still works? Obviously, you won't be able to select "no group", but as long as the db still works, we should be good.

It should still work.

If that happened the only difference would be group_id would remain nullable.

   $table->unsignedInteger('group_id')->default(1)->comment('The id of the user group.')->change();
@lcharette

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Only issue I see so far is in the user detail page :

Capture d’écran, le 2019-07-10 à 20 52 24

The separating the username and group makes it weird now. Simple if in Twig should fix that.

@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Only issue I see so far is in the user detail page :

Capture d’écran, le 2019-07-10 à 20 52 24

The separating the username and group makes it weird now. Simple if in Twig should fix that.

Do we want this to show "No group" or just remove if they have that option select?

@lcharette

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Do we want this to show "No group" or just remove if they have that option select?

I think removing it by default makes more sense if someone doesn't want to use groups at all.

amosfolz added some commits Jul 11, 2019

Update user.html.twig
Hide group name if user is not in a group.
@amosfolz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Only issue I see so far is in the user detail page :

Capture d’écran, le 2019-07-10 à 20 52 24

The separating the username and group makes it weird now. Simple if in Twig should fix that.

Fixed in 3467690.

I think this will need to be a non-reversible change. I have been trying for hours to get a down migration to work that will set the column back to not null and have had no success.

If the change in the db is not reversed, and the files are reversed to 4.2.x, does it still works? Obviously, you won't be able to select "no group", but as long as the db still works, we should be good.

I retested and can confirm the DB still works fine in this situation.

@lcharette

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I retested and can confirm the DB still works fine in this situation.

I did too last and it does works fine.

@lcharette lcharette merged commit 161ee0f into userfrosting:develop Jul 12, 2019

4 checks passed

codecov/patch 88.88% of diff hit (target 66.93%)
Details
codecov/project 66.96% (+0.02%) compared to 61da177
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@amosfolz amosfolz deleted the amosfolz:issue-#867 branch Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.