[DEPRECATED PR]: structure flow to create the assignment #39
Conversation
…flextensions into 187434223-set-up-flow-to-add-lms-to-course
…434223-set-up-flow-to-add-lms-to-course
Zzz212zzZ
force-pushed
the
187434339-structure-flow-to-create-the-assignment
branch
from
cab3d24 to
569dc2e
Compare
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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[: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 |
There was a problem hiding this comment.
Lets also use the validation library for these as well
| t.bigint "course_to_lms_id", null: false | ||
| t.index ["course_to_lms_id"], name: "index_assignments_on_course_to_lms_id" |
There was a problem hiding this comment.
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?
| 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.
If this is supposed to be a reference to a join table, it should prob be a foreign key only
…434339-structure-flow-to-create-the-assignment
Zzz212zzZ
changed the title
General Info
This is a deprecated pr messed up with many unrelated commits. So I created a new branch and made a new pr with clean commits, plus amended the code according to the comments in this deprecated pr.
Changes
Explain your changes here (in such a way that you would understand why you made them a year from now).
AssignmentTable. Now it should rely onCourse_to_lmsTable instead oflmsTable.createmethod within the assignment controllerTesting
courseandlmsexist before. It uses a foreign key ofcourse_to_lms_idto ensure this.course_to_lms_idandexternal_assignment_idthat we want to create.Documentation
Updated E-R diagram
Checklist