Add groups to courses #11

machadovilaca · 2018-07-23T18:52:15Z

This pull request allows work groups creation.
Admins can setup courses group sizes and the group creation period, they can also view existing groups for each course.
Students can create groups for courses with minimum group size bigger than zero, after group creation they can invite other students to join his group. Invitations are not limited but the number of group elements can not exceed maximum defined group size.

fntneves · 2018-07-24T10:31:32Z

Did you implement tests for this feature?
If not, this is why the "cross" is appearing. However, I will look at it for a first review and then you work on tests.

machadovilaca · 2018-07-24T13:28:44Z

@fntneves, no we did not. I can work on them during the next days, any tips for how to write them? What features should I test? For example, can I in a single test verify group creation, invitations, accepting and refusing that invitation and test group size limite?

machadovilaca · 2018-07-24T13:37:42Z

app/Http/Requests/Settings/UpdateRequest.php

@@ -28,6 +28,8 @@ public function rules()
            'enrollments_end_at' => 'required|date|after:enrollments_start_at',
            'exchanges_start_at' => 'required|date|after_or_equal:enrollments_start_at',
            'exchanges_end_at' => 'required|date|after:exchanges_start_at',
+            //'groups_creation_start_at' => 'required|date|after_or_equal:exchanges_end_at',


@fntneves I left this commented for testing purposes but anyway we need to discuss this, I thought this restriction would fit the feature but want to know your opinion, probably should change "after_or_equal" to "after" though.

fntneves · 2018-09-04T10:40:44Z

Tests are failing, can you check what is going on?

machadovilaca · 2018-09-07T19:19:52Z

@fntneves Resolvido, tenho de ir adicionando o resto agora

djcouto

First of all great work! It's a big MR to a project that brings a really important and needed feature.
My firsts comments are very similar, they are related to missing database transactions, some code refactor and request validation files, etc.

After you resolve this questions I will come back a new review.
I take a quick lock at the tests and I noticed that there a few features tests missing. You should make tests to each store, update and destroy method that you have created. Also, I noticed you have an exception but don't have a test for it, so that is missing too. Notice that the code coverage reduce 15% so, there stills some work to do 😄

Read the comments and discuss with me any problem or any question that you have any doubt or don't agree with. I'll be watching this MR.

app/Http/Controllers/Admin/CourseController.php

database/migrations/2018_07_13_232043_create_memberships_table.php

database/migrations/2018_07_21_144829_create_invitations_table.php

public/css/app.css

app/Exceptions/UserHasAlreadyAnInviteInGroupException.php

app/Exceptions/UserHasAlreadyGroupInCourseException.php

app/Http/Controllers/Admin/GroupController.php

app/Http/Controllers/GroupController.php

tests/Feature/InvitationTest.php

tests/Unit/GroupTest.php

resources/views/admin/groups/index.blade.php

djcouto · 2018-09-24T12:20:22Z

app/Http/Controllers/GroupController.php

+     *
+     * @return \Illuminate\View\View
+     */
+    public function index()


This method really needs a refactor. From where I see it this is what this method needs:

Get the courses which the authenticated student has enrolled. ( Student::enrolledCourses )

For each course get only available groups, right? ( Course::availableGroups )

For each group get the number of invitations, that is being correctly done.

What do you think?

We should discuss this personally

…er update method

…obust

djcouto · 2018-10-08T08:55:12Z

tests/Unit/InvitationTest.php

+    /** @test */
+    public function testStudentHasOnlyOneInvitationToEachGroup()
+    {
+        $student = factory(Student::class)->create();


add this at the top and remove the try and catch statement
$this->expectException(UserHasAlreadyAnInviteInGroupException::class)

djcouto · 2018-10-08T08:56:31Z

tests/Unit/MembershipTest.php

+
+    public function testStudentHasOnlyOneMembershipPerCourse()
+    {
+        $student = factory(Student::class)->create();


add this at the top and remove the try and catch statement
$this->expectException(UserHasAlreadyGroupInCourseException::class)

djcouto · 2018-10-08T09:15:35Z

app/Providers/AppServiceProvider.php

@@ -29,6 +29,18 @@ public function boot()
        Validator::extend('student_number', function ($attribute, $value, $parameters, $validator) {
            return preg_match('/^(a|pg)[0-9]+$/i', $value) === 1;
        });
+
+        Validator::extend('greater_or_equal_than_field', function ($attribute, $value, $parameters, $validator) {


This is how this validation is implemented in laravel 5.7
Maybe you should take a look to make this method similar, just to guarantee we are not missing any case.

https://github.com/laravel/framework/blob/ac745730492ef23a04ce614228d11e496feb625d/src/Illuminate/Validation/Concerns/ValidatesAttributes.php#L879

djcouto · 2018-10-08T09:16:53Z

app/Providers/AppServiceProvider.php

+            return $value >= $min_value;
+        });
+
+        Validator::replacer('greater_or_equal_than_field', function ($message, $attribute, $rule, $parameters) {


Same as in the above comment.
https://github.com/laravel/framework/blob/adc49acbefe26f03cd8740003d17d7baf154626e/src/Illuminate/Validation/Concerns/ReplacesAttributes.php#L287

djcouto · 2018-10-08T09:21:04Z

app/Http/Controllers/GroupController.php

+            return $group;
+        });
+
+        if ($group == null) {


This case never happens, remove it.

djcouto · 2018-10-08T09:35:03Z

app/Http/Controllers/GroupController.php

+            flash('You have successfully joined a group.')->success();
+        } catch (UserHasAlreadyGroupInCourseException $e) {
+            flash('You already have a group.')->error();
+            $group->delete();


Creating a record and delete it? 2 queries? Hum...
You should check for the existing a group in this course which this user as membership. In this way, this exception is no longer need because the validation is done before...
But what do you think? Is this work worthy?

hmatalonga

I have added a couple of suggestions, feel free to skip them if they are not doable at this stage...

hmatalonga · 2018-10-08T09:59:36Z

app/Http/Controllers/Admin/CourseController.php

+    public function update(UpdateRequest $request, $id)
+    {
+        DB::transaction(function () use ($request, $id) {
+            $course = Course::findOrFail($id);


@djcouto What would you say about using implicit binding for all these resource methods?

This way we could skip calling findOrFail altogether or even resolve some specific case via middleware if necessary.

So what do you think would be beneficial at this stage applying this refactor?

This is simply a suggestion that can be taken in consideration for every controller 😉

I think is a good suggestion, it cleans some code. My only concerns are if it will get a little messy at the RouteServiceProvider, but for now, it seems a good approach.
If you choose to do this refactor please don't forget to customize the resolution logic using the method Route::bind so you can assure that it runs inside a transaction.

hmatalonga · 2018-10-08T10:03:43Z

app/Http/Controllers/Admin/CourseController.php

+        DB::transaction(function () use ($request, $id) {
+            $course = Course::findOrFail($id);
+
+            $course->fill($request->all());


Another suggestion of the same nature... Mostly to reduce the length of the code 😄

Instead of these:

$course->fill($request->all()); $course->save();

To this:

$course = tap($course)->update($request->all());

machadovilaca requested a review from fntneves July 23, 2018 18:52

fntneves changed the title ~~Work Groups~~ Add groups to courses Jul 24, 2018

machadovilaca commented Jul 24, 2018

View reviewed changes

machadovilaca force-pushed the pm-jv-work_groups branch from 45d15e6 to 0766472 Compare August 29, 2018 09:49

machadovilaca force-pushed the pm-jv-work_groups branch 3 times, most recently from 9656e37 to 74f4745 Compare September 7, 2018 19:03

djcouto self-requested a review September 10, 2018 10:59

djcouto requested changes Sep 11, 2018

View reviewed changes

machadovilaca force-pushed the pm-jv-work_groups branch 3 times, most recently from 7e3269c to 25367dc Compare September 16, 2018 14:38

djcouto requested a review from hmatalonga September 22, 2018 15:26

djcouto requested changes Sep 24, 2018

View reviewed changes

machadovilaca force-pushed the pm-jv-work_groups branch 2 times, most recently from 7032a57 to 53b84c9 Compare October 5, 2018 19:36

machadovilaca and others added 10 commits October 5, 2018 20:37

Add group related routes, controllers and models

b9dd8ad

Update routes

8955379

Rename add_group_size_to_courses migration

ebb12be

Update groups backend and add views

7b0b697

Add group creation period to settings

c220057

Update admin course controller

8056583

Add membership to link groups and students

08c76c4

Update student model

a6b5b5f

Add invitations

3633ccd

Update controllers and relationships

e6aff3a

machadovilaca added 14 commits October 5, 2018 20:37

Add settings tests

1cb99d6

Add unit tests

d2fc981

Add Group tests

5384bfb

Add db transaction and FormRequest validation to Admin\CourseControll…

a544d66

…er update method

Inline variable in UserHasAlreadyGroupInCourseException

ce6a3a2

Use eagerloading on Admin\GroupController show method

0218d6f

Use db transactions and eagerloading on several GroupController methods

acf7f86

Revert encapsulating not unary operator argument

921c3c9

Add db transaction, FormRequests and make InvitationController more r…

d972c90

…obust

Fix style related issues

7d5f352

Add DatabaseTransactions to Membership test

b3df65b

Improve test coverage

a898c6c

Update exceptions

97bd577

Update test and accept invite controller

70a6a87

machadovilaca force-pushed the pm-jv-work_groups branch from 53b84c9 to 70a6a87 Compare October 5, 2018 19:38

Add groups seeder

ac14f29

machadovilaca force-pushed the pm-jv-work_groups branch from bd8ea13 to ac14f29 Compare October 6, 2018 13:44

djcouto requested changes Oct 8, 2018

View reviewed changes

hmatalonga requested changes Oct 8, 2018

View reviewed changes

fntneves closed this Jun 21, 2019

fntneves deleted the pm-jv-work_groups branch July 2, 2019 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add groups to courses #11

Add groups to courses #11

machadovilaca commented Jul 23, 2018

fntneves commented Jul 24, 2018

machadovilaca commented Jul 24, 2018

machadovilaca Jul 24, 2018

fntneves commented Sep 4, 2018

machadovilaca commented Sep 7, 2018 •

edited

Loading

djcouto left a comment •

edited

Loading

djcouto Sep 24, 2018

machadovilaca Oct 5, 2018

djcouto Oct 8, 2018

djcouto Oct 8, 2018

djcouto Oct 8, 2018

djcouto Oct 8, 2018

djcouto Oct 8, 2018

djcouto Oct 8, 2018

hmatalonga left a comment

hmatalonga Oct 8, 2018

djcouto Oct 9, 2018

hmatalonga Oct 8, 2018

djcouto Oct 9, 2018

Add groups to courses #11

Add groups to courses #11

Conversation

machadovilaca commented Jul 23, 2018

fntneves commented Jul 24, 2018

machadovilaca commented Jul 24, 2018

Choose a reason for hiding this comment

fntneves commented Sep 4, 2018

machadovilaca commented Sep 7, 2018 • edited Loading

djcouto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmatalonga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadovilaca commented Sep 7, 2018 •

edited

Loading

djcouto left a comment •

edited

Loading