Controller routes #89

Kelisnor · 2022-08-05T18:18:10Z

//There's no specific page for course and assignment, so I use AdminPage instead

…ation

…foreign key for Assignment

…e by admin

cjreed121 · 2022-08-05T18:21:02Z

app/Http/Controllers/AssignmentController.php

+    public function create(User $user): RedirectResponse {
+        if ($user->is_admin) {
+            return redirect()->route('assignment.index')->with('success', 'Assignment created');
+        }
+        return redirect()->route('assignment.index')->with('error', 'User is not an admin');
+    }


This doesn't make an assignment which it should. Also any user should be able to make an assignment for their class rather than just admins. Your policies should control whether a user can access or not.

cjreed121 · 2022-08-05T18:21:38Z

app/Http/Controllers/AssignmentController.php

+    public function index()
+    {
+        return inertia("AdminPage");
+    }
+


Just leave this function empty for now rather than render a page that is unrelated.

cjreed121 · 2022-08-05T18:22:24Z

app/Http/Controllers/AssignmentController.php

+    public function update(User $user): RedirectResponse {
+        if ($user->is_admin) {
+            return redirect()->route('assignment.index')->with('success', 'Assignment updated');
+        }
+        return redirect()->route('assignment.index')->with('error', 'User is not an admin');
+    }
+
+
+    public function destroy(User $user): RedirectResponse {
+        if ($user->is_admin) {
+            return redirect()->route('assignment.index')->with('success', 'Assignment deleted');
+        }
+        return redirect()->route('assignment.index')->with('error', 'User is not an admin');
+    }


These don't make any database changes and shouldn't check for admin.

cjreed121 · 2022-08-05T18:22:47Z

app/Http/Controllers/AssignmentController.php

+    public function findAssignmentByCourse($course_id)
+    {
+        $assignments_list = Assignment::where('course_id', $course_id)->get();
+        foreach ($assignment as $assignments_list) {
+            $assignment->name=$assignments_list->assignment->name;
+        }
+        return response()->json(['data'=>$assignment]);
+    }


Doesn't seem to be used?

Can be removed since still not used

cjreed121 · 2022-08-05T18:23:01Z

app/Http/Controllers/CourseController.php

+    public function index() {
+        return inertia("AdminPage");
+    }


This can be left empty.

cjreed121 · 2022-08-05T18:26:56Z

app/Policies/AssignmentPolicy.php

+    public function update(User $user, Assignment $assignment)
+    {
+        return $use->is_admin
+        ? Response::allow()
+        : Response::deny();
+    }
+
+    /**
+     * Determine whether the user can delete the model.
+     *
+     * @param  \App\Models\User  $user
+     * @param  \App\Models\Assignment  $assignment
+     * @return \Illuminate\Auth\Access\Response|bool
+     */
+    public function delete(User $user, Assignment $assignment)
+    {
+        return $use->is_admin
+        ? Response::allow()
+        : Response::deny();
+    }


These should check if the user owns the assignment instead.

cjreed121 · 2022-08-05T18:27:54Z

app/Policies/CoursePolicy.php

+    public function view(User $user, Course $course)
+    {
+        return true;
+    }
+
+    /**
+     * Determine whether the user can create models.
+     *
+     * @param  \App\Models\User  $user
+     * @return \Illuminate\Auth\Access\Response|bool
+     */
+    public function create(User $user)
+    {
+        return $use->is_admin
+        ? Response::allow()
+        : Response::deny();
+    }
+
+    /**
+     * Determine whether the user can update the model.
+     *
+     * @param  \App\Models\User  $user
+     * @param  \App\Models\Course  $course
+     * @return \Illuminate\Auth\Access\Response|bool
+     */
+    public function update(User $user, Course $course)
+    {
+        return $use->is_admin
+        ? Response::allow()
+        : Response::deny();
+    }
+
+    /**
+     * Determine whether the user can delete the model.
+     *
+     * @param  \App\Models\User  $user
+     * @param  \App\Models\Course  $course
+     * @return \Illuminate\Auth\Access\Response|bool
+     */
+    public function delete(User $user, Course $course)
+    {
+        return $use->is_admin
+        ? Response::allow()
+        : Response::deny();
+    }   


Should check if user owns the course

cjreed121 · 2022-08-05T18:28:17Z

app/Providers/AuthServiceProvider.php

+        Assignment::class => AssignmentPolicy::class,
+        Course::class => CoursePolicy::class,


Laravel should automatically register these. Was it not doing so?

cjreed121 · 2022-08-05T18:28:40Z

app/Providers/AuthServiceProvider.php

+        Gate::define('update-post', function (User $user, 
+        Course $course, Assignment $assignment) {
+            //?check if user is an admin
+            return ($assignment->course_id === $course->id);
+        });
+


What is this gate for?

cjreed121 · 2022-08-05T18:29:12Z

routes/web.php

+    Route::apiResource('users', CourseController::class)->only(['create','update', 'destroy']);
+    Route::apiResource('users', AssignmnetController::class)->only(['create','update', 'destroy']);


The names should change and these shouldn't be in the admin middleware

cjreed121

I have some review comments. Also if you can add some tests for these that would be great.

cjreed121 · 2022-08-12T18:33:40Z

app/Http/Controllers/AssignmentController.php

+    public function create(){
+        return Redirect::to('assignment.index');
+    }
+


This should actually create the assignment here not just redirect.

cjreed121 · 2022-08-12T18:34:06Z

app/Http/Controllers/AssignmentController.php

+        return response()->json(['data'=>$assignment]);
+    }
+
+    public function findAssignmentByUser($user_id)


Not used so can be removed

cjreed121 · 2022-08-12T18:34:23Z

app/Http/Controllers/AssignmentController.php

+    public function findAssignmentByCourse($course_id)
+    {
+        $assignments_list = Assignment::where('course_id', $course_id)->get();
+        foreach ($assignment as $assignments_list) {
+            $assignment->name=$assignments_list->assignment->name;
+        }
+        return response()->json(['data'=>$assignment]);
+    }


Can be removed since still not used

cjreed121 · 2022-08-12T18:36:15Z

app/Http/Controllers/AssignmentController.php

+    public function show($id)
+    {
+        $assignment = Assignment::find($id);
+
+        //return View::make('')->with('assignment', $assignment);
+    }


If this is properly type hinted with an Assignment model then Laravel will find the model and you can just handle returning an inertia view with that model.

cjreed121 · 2022-08-12T18:36:48Z

app/Http/Controllers/AssignmentController.php

+    public function update($id){
+        $assignment = Assignment::find($id);
+        $assignment->name = Input::get('name');
+        $assignment->due_at = Input::get('due_at');
+
+        return Redirect::to('assignment.index');


We should really have validation here. You can make a request class to separate this logic as well.

cjreed121 · 2022-08-12T18:39:49Z

app/Models/Assignment.php

+    public function user(){
+        return $this->belongsTo(User::class);
+    }


This likely won't be too useful so it can be removed

cjreed121 · 2022-08-12T18:40:21Z

app/Models/User.php

+    public function assignments(){
+        return $this->hasMany(Assignment::class);
+    }


This should be hasManyThrough with the Assignment and Course passed in

cjreed121 · 2022-08-12T18:41:14Z

database/migrations/2022_06_17_181143_create_courses_table.php

@@ -14,6 +14,7 @@ public function up() {
        Schema::create('courses', function (Blueprint $table) {
            $table->id();
            $table->string('name');
+            $table->foreignId('user_id')->constrained()->onDelete('cascade');


We don't want to edit existing migrations

cjreed121 · 2022-08-12T18:41:23Z

database/migrations/2022_06_17_181539_create_assignments_table.php

@@ -13,6 +13,7 @@
    public function up() {
        Schema::create('assignments', function (Blueprint $table) {
            $table->id();
+            $table->foreignId('user_id')->constrained()->onDelete('cascade');


We don't want to edit existing migrations

cjreed121 · 2022-08-12T18:42:12Z

routes/web.php

+    Route::get('/course',[CourseController::class,'index'])->name('course.index');
+    Route::get('/assignment',[AssignmentController::class,'index'])->name('assignment.index');


These should be resource instead of get so that all of the routes become available

Kelisnor added 27 commits June 17, 2022 14:44

add simple models and migrations for course and assignment

6f2c1b5

Merge branch 'master' of https://github.com/ready-app/ready

8cf4beb

Merge branch 'master' of https://github.com/ready-app/ready

58109d3

add more things for models and migrations for course and assignment

c07c74c

fix a casting problem

545b382

Merge branch 'master' of https://github.com/ready-app/ready

0c65323

using = true to attempt generating created_at and update_at

48a58bc

Merge branch 'master' of https://github.com/ready-app/ready

0b4cdd3

delete manully creating of 'created_at' attribute for Assignment migr…

1a1e1c1

…ation

adding relationships between Assignment and Course class, setting up …

17c7944

…foreign key for Assignment

delete unnecessary attribute in Assignment model

0608652

Merge branch 'master' of https://github.com/ready-app/ready

3ba87e5

Merge branch 'AssignmentModelAndMigration'

91c0c8f

foreign key changes

b7d5f04

basic controller for Course

90ebb8c

adding some policies and gates

71c660b

single change to routes

01bdcba

add assignment controller

e73eccb

Merge branch 'master' into ControllerRoutes

32d1648

rewirte the update and destroy function so that it could only be chag…

cee750c

…e by admin

delete the original CourseWork Controller

c021397

minor changes to controllers

b89da19

delete unused functions

a56539b

rewrite functions with is_admin attribute

4e05200

minor changes

e3d8823

changes in policies

e80233b

minor changes to delete unused functions

b94a8df

cjreed121 requested changes Aug 5, 2022

View reviewed changes

change to the relations between Models

5028d01

Kelisnor requested a review from cjreed121 August 12, 2022 18:32

cjreed121 requested changes Aug 12, 2022

View reviewed changes

Kelisnor added 2 commits August 13, 2022 23:44

add validations

1307f0a

change to resource controller

2203483

Kelisnor requested a review from cjreed121 August 14, 2022 04:08

minor changes

c47f35d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller routes #89

Controller routes #89

Kelisnor commented Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 Aug 5, 2022

cjreed121 left a comment

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

cjreed121 Aug 12, 2022

		Assignment::class => AssignmentPolicy::class,
		Course::class => CoursePolicy::class,

		Route::apiResource('users', CourseController::class)->only(['create','update', 'destroy']);
		Route::apiResource('users', AssignmnetController::class)->only(['create','update', 'destroy']);

		Route::get('/course',[CourseController::class,'index'])->name('course.index');
		Route::get('/assignment',[AssignmentController::class,'index'])->name('assignment.index');

Controller routes #89

Are you sure you want to change the base?

Controller routes #89

Conversation

Kelisnor commented Aug 5, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjreed121 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

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