-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DEPRECATED PR]: structure flow to create the assignment #39
Changes from 13 commits
16f6a26
f48fa29
bb98d3d
03264c7
99ff5e0
5b4a9e9
08ed513
6b6f81f
68b45e1
4cb4592
f7a2333
694494f
569dc2e
c66a060
90f7f9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,59 @@ | ||
module Api | ||
module V1 | ||
class AssignmentsController < ApplicationController | ||
module V1 | ||
class AssignmentsController < ApplicationController | ||
before_action :validate_ids!, only: [:create] | ||
|
||
def index | ||
render json: 'not yet implemented', status: 501 | ||
def index | ||
render json: 'not yet implemented', status: 501 | ||
end | ||
|
||
# POST /courses/:course_id/lms/:lms_id/assignments | ||
def create | ||
course_to_lms = fetch_course_to_lms(params[:course_id], params[:lms_id]) | ||
return unless course_to_lms | ||
|
||
return if assignment_exists?(course_to_lms, params[:name], params[:external_assignment_id]) | ||
|
||
create_and_render_assignment(course_to_lms, params[:name], params[:external_assignment_id]) | ||
end | ||
|
||
def destroy | ||
render json: 'not yet implemented', status: 501 | ||
end | ||
|
||
private | ||
|
||
def fetch_course_to_lms(course_id, lms_id) | ||
course_to_lms = CourseToLms.find_by(course_id: course_id, lms_id: lms_id) | ||
unless course_to_lms | ||
render json: { error: 'No such Course_LMS association' }, status: :not_found | ||
end | ||
course_to_lms | ||
end | ||
|
||
def create | ||
render json: 'not yet implemented', status: 501 | ||
def assignment_exists?(course_to_lms, assignment_name, external_assignment_id) | ||
existing_assignment = Assignment.find_by(course_to_lms_id: course_to_lms.id, name: assignment_name, external_assignment_id: external_assignment_id) | ||
if existing_assignment | ||
render json: { message: 'Record already exists' }, status: :ok | ||
return true | ||
end | ||
false | ||
end | ||
|
||
def destroy | ||
render json: 'not yet implemented', status: 501 | ||
def create_and_render_assignment(course_to_lms, assignment_name, external_assignment_id) | ||
assignment = Assignment.new(course_to_lms_id: course_to_lms.id, name: assignment_name, external_assignment_id: external_assignment_id) | ||
if assignment.save | ||
render json: assignment, status: :created | ||
else | ||
render json: assignment.errors, status: :unprocessable_entity | ||
end | ||
end | ||
Comment on lines
+44
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say it might be better to include this in the controller method logic itself since it renders stuff. Since the method is clear about the fact that it renders, it makes it a bit clearer, but I still tend to avoid it since it is "side-effecty" |
||
|
||
def validate_ids! | ||
if params[:name].blank? || params[:external_assignment_id].blank? || params[:course_id].blank? || params[:lms_id].blank? | ||
render json: { error: 'Params required' }, status: :bad_request | ||
end | ||
end | ||
Comment on lines
+53
to
57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also added a validation class to do some of this validation as well (eg: ensuring that they are integers). The reason why this is important is because we need to make sure we are not introducing any vulnerabilities with user provided input. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,45 @@ | ||
module Api | ||
module V1 | ||
class LmssController < BaseController | ||
before_action :validate_name!, only: [:create] | ||
before_action :validate_ids!, only: [:create] | ||
|
||
# POST /courses/:course_id/lmss | ||
def create | ||
render :json => 'not yet implemented'.to_json, status: 501 | ||
course_id = params[:course_id] # course_id from the URL | ||
lms_id = params[:lms_id] # lms_id from the body | ||
external_course_id = params[:external_course_id] # external_course_id from the body | ||
|
||
# Ensure the course exists | ||
unless Course.exists?(course_id) | ||
render json: { error: 'Course not found' }, status: :not_found | ||
return | ||
end | ||
|
||
# Ensure the lms exists | ||
unless Lms.exists?(lms_id) | ||
render json: { error: 'Lms not found' }, status: :not_found | ||
return | ||
end | ||
|
||
# Check if the entry already exists | ||
existing_entry = CourseToLms.find_by(course_id: course_id, lms_id: lms_id, external_course_id: external_course_id) | ||
if existing_entry | ||
render json: { message: 'The association between the specified course and LMS already exists.' }, status: :ok | ||
return | ||
end | ||
|
||
# Create the course_to_lms entry | ||
course_to_lms = CourseToLms.new( | ||
course_id: course_id, | ||
lms_id: lms_id, | ||
external_course_id: external_course_id | ||
) | ||
|
||
if course_to_lms.save | ||
render json: course_to_lms, status: :created | ||
else | ||
render json: course_to_lms.errors, status: :unprocessable_entity | ||
end | ||
end | ||
|
||
def index | ||
|
@@ -20,9 +55,9 @@ def destroy | |
# TODO: this should be exported to its own (validator) class. | ||
# TODO: this validation should also check the config file for the name of lms's. | ||
# | ||
def validate_name! | ||
if params['name'].blank? | ||
render :json => 'name parameter is required', status: 401 | ||
def validate_ids! | ||
if params[:course_id].blank? || params[:lms_id].blank? || params[:external_course_id].blank? | ||
render json: { error: 'course_id and lms_id are required' }, status: :bad_request | ||
Comment on lines
+58
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets also use the validation library for these as well |
||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# app/models/assignment.rb | ||
class Assignment < ApplicationRecord | ||
belongs_to :course_to_lms | ||
|
||
validates :name, presence: true | ||
validates :external_assignment_id, presence: true | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# app/models/course.rb | ||
|
||
class Course < ApplicationRecord | ||
|
||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# app/models/course_to_lms.rb | ||
class CourseToLms < ApplicationRecord | ||
# Association with Course and Lms | ||
belongs_to :course | ||
belongs_to :lms | ||
|
||
# You can add validations here if needed, for example: | ||
validates :course_id, presence: true | ||
validates :lms_id, presence: true | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# app/models/lms.rb | ||
class Lms < ApplicationRecord | ||
|
||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
class ChangeLmsIdToCourseToLmsIdInAssignments < ActiveRecord::Migration[7.1] | ||
def change | ||
remove_index :assignments, :lms_id | ||
remove_column :assignments, :lms_id, :bigint | ||
add_column :assignments, :course_to_lms_id, :bigint, null: false | ||
add_index :assignments, :course_to_lms_id | ||
add_foreign_key :assignments, :course_to_lmss, column: :course_to_lms_id | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,17 +10,17 @@ | |
# | ||
# It's strongly recommended that you check this file into your version control system. | ||
|
||
ActiveRecord::Schema[7.1].define(version: 2024_04_20_211809) do | ||
ActiveRecord::Schema[7.1].define(version: 2024_04_20_232708) do | ||
# These are extensions that must be enabled in order to support this database | ||
enable_extension "plpgsql" | ||
|
||
create_table "assignments", force: :cascade do |t| | ||
t.bigint "lms_id" | ||
t.string "name" | ||
t.datetime "created_at", null: false | ||
t.datetime "updated_at", null: false | ||
t.string "external_assignment_id" | ||
t.index ["lms_id"], name: "index_assignments_on_lms_id" | ||
t.bigint "course_to_lms_id", null: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is supposed to be a reference to a join table, it should prob be a foreign key only |
||
t.index ["course_to_lms_id"], name: "index_assignments_on_course_to_lms_id" | ||
Comment on lines
+22
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assignment is owned by a course which is owned by an lms so shouldn't we use that relation to get this value instead of adding it to every assignment? |
||
end | ||
|
||
create_table "course_to_lmss", force: :cascade do |t| | ||
|
@@ -89,7 +89,7 @@ | |
t.index ["email"], name: "index_users_on_email", unique: true | ||
end | ||
|
||
add_foreign_key "assignments", "lmss" | ||
add_foreign_key "assignments", "course_to_lmss" | ||
add_foreign_key "course_to_lmss", "courses" | ||
add_foreign_key "course_to_lmss", "lmss" | ||
add_foreign_key "extensions", "assignments" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This render causes side effects which we always try to avoid. Instead, we should try to make all renders happen from the controller method which the route calls.