Assessments #34
Assessments #34
Conversation
lib/cadet/submission_status.ex
Outdated
| @@ -0,0 +1,5 @@ | |||
| import EctoEnum | |||
There was a problem hiding this comment.
I think this is a duplicate file, should be located in lib/cadet/assessments/ instead
lib/cadet/assessments/Assessment.ex
Outdated
| @@ -0,0 +1,29 @@ | |||
| defmodule Cadet.Assessments.Assessment do | |||
There was a problem hiding this comment.
I don't think we need assessment entity since all this does is has_one :mission, Mission, is redundant.
indocomsoft
left a comment
indocomsoft
left a comment
There was a problem hiding this comment.
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.
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.
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.
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 |
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 |
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 |
There was a problem hiding this comment.
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 |
| @@ -0,0 +1,5 @@ | |||
| import EctoEnum | |||
|
|
|||
| defenum Cadet.Assessments.ProblemType, :type, [ | |||
There was a problem hiding this comment.
For consistency The enum should be named QuestionType since it is the type of Question
There was a problem hiding this comment.
Enum must be used for question and answer I think
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.
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 | |||
indocomsoft
left a comment
indocomsoft
left a comment
There was a problem hiding this comment.
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 %{ |
| @@ -0,0 +1,77 @@ | |||
| defmodule CadetWeb.AssessmentController do | |||
There was a problem hiding this comment.
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 |
Pull Request Test Coverage Report for Build 486
💛 - Coveralls |
indocomsoft
left a comment
indocomsoft
left a comment
There was a problem hiding this comment.
- You still have not refactored all the json deserialisation as requested during the meeting
- Please check the warnings produced by the compiler -- there are many useless variables and
import,usestatements - Try to have only a single
assertfor each unit test
.gitignore
Outdated
| # Uploaded file | ||
| /uploads | ||
|
|
||
|
|
There was a problem hiding this comment.
Please remove duplicate blank line
lib/cadet/assessments.ex
Outdated
|
|
||
| import Ecto.Changeset | ||
| import Ecto.Query | ||
| import Cadet.ContextHelper |
There was a problem hiding this comment.
Move all the import statements to cadet.ex in the :context part instead
lib/cadet/assessments/mission.ex
Outdated
| use Cadet, :model | ||
| use Arc.Ecto.Schema | ||
|
|
||
| import Cadet.ModelHelper |
There was a problem hiding this comment.
Don't import again because this has already been imported in cadet.ex
lib/cadet/assessments/submission.ex
Outdated
|
|
||
| import Ecto.Query | ||
|
|
||
| alias Cadet.Assessments.Assessment |
lib/cadet/assessments/submission.ex
Outdated
| @moduledoc false | ||
| use Cadet, :model | ||
|
|
||
| import Ecto.Query |
There was a problem hiding this comment.
Please remove unused import. Model should not be doing any query anyway
test/cadet/assessments_test.exs
Outdated
| is_published: true | ||
| ) | ||
|
|
||
| result = Enum.map(Assessments.missions_due_soon(), fn m -> m.id end) |
There was a problem hiding this comment.
missions_due_ids is a better name
test/cadet/assessments_test.exs
Outdated
| assert mission.title == "changed_mission" | ||
| end | ||
|
|
||
| test "all missions with category" do |
There was a problem hiding this comment.
Please split each category into separate tests
test/cadet/assessments_test.exs
Outdated
| end | ||
|
|
||
| test "update question" do | ||
| mission = insert(:mission) |
test/cadet/assessments_test.exs
Outdated
| end | ||
|
|
||
| test "delete question" do | ||
| mission = insert(:mission) |
test/cadet/assessments_test.exs
Outdated
|
|
||
| result = Enum.map(Assessments.missions_due_soon(), fn m -> m.id end) | ||
| assert mission_in_timerange.id in result | ||
| refute mission_far.id in result |
There was a problem hiding this comment.
you need to add refute mission_before_now.id in result too
* 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.
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.
Good point. I'll write it down as a TODO
lib/cadet/assessments/assessments.ex
Outdated
| end | ||
|
|
||
| def create_assessment(params) do | ||
| changeset = build_assessment(params) |
There was a problem hiding this comment.
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.
i was thinking of
params
|> build_assessment()
|> Repo.insert()
lib/cadet/assessments/assessments.ex
Outdated
| end | ||
|
|
||
| def publish_assessment(id) do | ||
| assessment = get_assessment(id) |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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.
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