-
Notifications
You must be signed in to change notification settings - Fork 50
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
Assessments #34
Assessments #34
Conversation
lib/cadet/submission_status.ex
Outdated
@@ -0,0 +1,5 @@ | |||
import EctoEnum |
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.
I think this is a duplicate file, should be located in lib/cadet/assessments/
instead
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.
Bump
lib/cadet/assessments/Assessment.ex
Outdated
@@ -0,0 +1,29 @@ | |||
defmodule Cadet.Assessments.Assessment do |
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.
I don't think we need assessment
entity since all this does is has_one :mission, Mission
, is redundant.
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.
Hey, I added some more comments.
lib/cadet/assessments/Question.ex
Outdated
field :title, :string | ||
field :display_order, :integer | ||
field :weight, :integer | ||
field :question_json, :map |
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.
Following the convention in source-academy
, I think the field name should be :question
lib/cadet/assessments/Question.ex
Outdated
field :weight, :integer | ||
field :question_json, :map | ||
field :type, ProblemType | ||
field :raw_json, :string, virtual: true |
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.
And similarly this shuld be :raw_question
lib/cadet/assessments/Question.ex
Outdated
|> put_json | ||
end | ||
|
||
defp put_json(changeset) do |
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.
Similarly, this should be put_question
lib/cadet/assessments/answer.ex
Outdated
|
||
schema "answers" do | ||
field :marks, :float, default: 0.0 | ||
field :answer_json, :map |
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.
Same thing here
lib/cadet/assessments/answer.ex
Outdated
field :marks, :float, default: 0.0 | ||
field :answer_json, :map | ||
field :type, ProblemType | ||
field :raw_json, :string, virtual: true |
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.
And here
lib/cadet/assessments/mission.ex
Outdated
end | ||
|
||
@required_fields ~w(order category title open_at close_at)a | ||
@optional_fields ~w(summary_short summary_long) | ||
@required_fields ~w(name order category title open_at close_at library)a |
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.
Don't forget to remove from here too
lib/cadet/assessments/mission.ex
Outdated
@required_fields ~w(order category title open_at close_at)a | ||
@optional_fields ~w(summary_short summary_long) | ||
@required_fields ~w(name order category title open_at close_at library)a | ||
@optional_fields ~w(summary_short summary_long is_published max_xp priority raw_library)a |
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.
And here
@@ -0,0 +1,5 @@ | |||
import EctoEnum | |||
|
|||
defenum Cadet.Assessments.ProblemType, :type, [ |
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.
For consistency The enum should be named QuestionType
since it is the type of Question
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.
Enum must be used for question and answer I think
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.
Fair enough
lib/cadet/assessments/submission.ex
Outdated
end | ||
|
||
def validate_role(changeset, user, role) do | ||
validate_change(changeset, user, fn _, user -> |
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.
should be fn ^user, user
although I think you may want to reconsider the variable names
lib/cadet/submission_status.ex
Outdated
@@ -0,0 +1,5 @@ | |||
import EctoEnum |
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.
Bump
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.
Do plan out with swagger first before writing your controller, I think that way the code we produce will be of higher quality. Also as much as possible run mix format
, mix test
and mix credo
and fix any problem before pushing. Generally a good idea
|
||
alias Cadet.Assessments | ||
|
||
@tabs_map %{ |
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.
Why do we have tabs?
@@ -0,0 +1,77 @@ | |||
defmodule CadetWeb.AssessmentController do |
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.
Please write swagger specification so I can review your code better :)
Assessments.update_question(params["id"], params["question"]) | ||
end | ||
|
||
def delete(conn, %{"mission_id" => mission_id, "id" => id}) do |
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.
Unclosed def
Pull Request Test Coverage Report for Build 486
💛 - Coveralls |
* Improve contexts - Allow using multiple contexts - Cleaned up some imports in /lib/cadet/accessments * Raise exception when using context with invalid arguments * More housecleaning * Add comment to cadet context
@tuesmiddt I feel this should be ready to merge, but since I made the edits, I think it's good that you have a lookthrough. If everything is alright, I'll merge this tomorrow. |
def all_open_assessments(category) do | ||
now = Timex.now() | ||
|
||
assessment_with_category = Repo.all(from(a in Assessment, where: a.category == ^category)) |
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.
Can use the all_assessments(category)
method above but I think just leave it for now.
In general I think we should combine this and the Enum.filter
below into one query string so that the DB engine only gives us the results we want instead of fetching too much data into memory and then filtering it. Can leave that for a later refactor.
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.
Good point. I'll write it down as a TODO
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.
lib/cadet/assessments/assessments.ex
Outdated
end | ||
|
||
def create_assessment(params) do | ||
changeset = build_assessment(params) |
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.
Use pipeline syntax
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.
https://github.com/lexmag/elixir-style-guide#needless-pipeline
Basically for single function call, don't use pipeline syntax.
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.
i was thinking of
params
|> build_assessment()
|> Repo.insert()
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.
Ohh makes sense
lib/cadet/assessments/assessments.ex
Outdated
end | ||
|
||
def publish_assessment(id) do | ||
assessment = get_assessment(id) |
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.
Use pipeline syntax
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.
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.
how about
id
|> get_assessment()
|> change(%{is_published: true})
|> Repo.update()
lib/cadet/assessments/assessments.ex
Outdated
Repo.get(Assessment, id) | ||
end | ||
|
||
def create_question(params, assessment_id) do |
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.
I think can combine this with create_question_for_assessment
lib/cadet/assessments/question.ex
Outdated
end | ||
|
||
defp put_question(changeset) do | ||
{:ok, json} = Poison.decode(get_change(changeset, :raw_question) || "{}") |
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.
I think this is clearer if we split it into two lines or do this instead:
{:ok, json} =
changeset
|> get_change(:raw_question)
|> Kernel.||("{}")
|> Poison.decode()
* Added Question and Assessment * Added functionality for grading * Added assessments functionality * Fixed typo * Restructured assessments * Added assessments context * Added controllers * Delete duplicate file * Changes to suit convention * Fixed stuff * Ran mix commands * Fix changes * Removed role validation * Removed all compile time errors * Fixed consistency issues * Refactored * Conversion to spaces * More functionality for questions * Adding tests * Refactored * Added few tests for missions * Few more tests to go * Added more tests * Delete useless migration * Delete useless migration 2 * Added submission test * Added question tests and switched controllers to new branch * Added answers test * Remove unneccesary stuff * Formatting * Fixed compile error * Make changes according to review * Fixes * Added tests to question and answer * Added question types * Various fixes * Changes according to question types * Assessments context * Fixes * Removed invalid test * Fix some tests * Assessments housecleaning (#51) * Improve contexts - Allow using multiple contexts - Cleaned up some imports in /lib/cadet/accessments * Raise exception when using context with invalid arguments * More housecleaning * Add comment to cadet context * Fix credo * Remove duplicate blank line * Remove pre-commit * Refactor json and rename Mission to Assessment * Fix submission model * Use cadet context * Fix some json tests * Fix tests and logic error * Remove untested method and add one test * Remove useless import * Add test * Fix according to code review * Change to pipeline syntax * Reorganise file
Fixes #26 #28