Refactor assessments context #102

tuesmiddt · 2018-07-08T12:01:38Z

Fixes #98
Fixes #58

Remove some unnecessary helper methods
Refactor assessments methods to use only 1 db query where possible
Refactored some tests

coveralls · 2018-07-08T12:04:19Z

Pull Request Test Coverage Report for Build 943

8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.02%) to 99.413%

Totals
Change from base Build 932:	-0.02%
Covered Lines:	508
Relevant Lines:	511

💛 - Coveralls

indocomsoft

Looks good overall, just some styling issues

indocomsoft · 2018-07-08T13:13:38Z

lib/cadet/assessments/assessments.ex

-    Repo.all(Assessment)
-  end
-
  def all_assessments(assessment_type) do


I think we should wait until my PR #101 is merged cos I added max_xp virtual field as well as query to fetch that.

sure lets do that

Do you wanna add the max_xp field here?

tbh i'm thinking of removing these methods. nothing's using them right now

Haha yes. I don't think anyone would miss it

Actually I'm thinking if it's possible to refactor Assessments context such that we can remove the assessments_test.exs altogether and getting coverage just from the controller tests. What do you think?

We can do away with a lot of them but we still need the functions that build questions/assessments

indocomsoft · 2018-07-08T13:13:59Z

lib/cadet/assessments/assessments.ex

  def all_assessments(assessment_type) do
-    Repo.all(from(a in Assessment, where: a.type == ^assessment_type))
+    Assessment
+    |> where([a], a.type == ^assessment_type)


|> where(type: ^assessment_type)

indocomsoft · 2018-07-08T13:14:26Z

lib/cadet/assessments/assessments.ex

-    # TODO: Refactor to be done on SQL instead of in-memory
-    Enum.filter(assessment_with_type, &(&1.is_published and Timex.before?(&1.open_at, now)))
+    Assessment
+    |> where([a], a.is_published)


Same thing, use the shorter where wherever possible

indocomsoft · 2018-07-08T13:19:23Z

lib/cadet/assessments/assessments.ex

-    Enum.filter(assessment_with_type, &(&1.is_published and Timex.before?(&1.open_at, now)))
+    Assessment
+    |> where([a], a.is_published)
+    |> where([a], a.open_at <= from_now(1, "second"))


why not 0 second lol

oh just wanted to give a little buffer, although I guess it makes no difference either way

indocomsoft · 2018-07-08T13:21:21Z

lib/cadet/assessments/assessments.ex

  end

  def create_question_for_assessment(params, assessment_id)
      when is_binary(assessment_id) or is_number(assessment_id) do


My bad, should be is_integer(assessment_id) instead of is_number

indocomsoft · 2018-07-08T13:23:28Z

lib/cadet/assessments/assessments.ex

+      assessment =
+        Assessment
+        |> where([assessment], assessment.id == ^assessment_id)
+        |> join(


Preferable to use one letter bindings, i.e.
|> join(:left, [a], q in Question, q.assessment_id == a.id)

indocomsoft · 2018-07-08T13:23:40Z

lib/cadet/assessments/assessments.ex

-      questions = assessment.questions
+      assessment =
+        Assessment
+        |> where([assessment], assessment.id == ^assessment_id)


Use the short where

indocomsoft · 2018-07-08T13:27:28Z

test/cadet/assessments/assessments_test.exs

-    result = Enum.map(Assessments.all_assessments(), & &1.id)
-    assert result == assessments
-  end
+  alias Cadet.Assessments.{Question, Assessment, AssessmentType}


I'm OCD but prefer alphabetical order {Assessment, AssessmentType, Question}

tuesmiddt · 2018-07-08T14:33:36Z

Made amendments as requested. Will wait on your controller before checking and merging again.

indocomsoft · 2018-07-15T14:24:08Z

Lel might wanna rebase/merge this on/with `master cos I'm pretty sure there'll be quite a number of conflicts @tuesmiddt

indocomsoft

Some refactor might be required.

Also wondering if we should do from_now(0, "second") instead of 1

indocomsoft · 2018-07-15T15:24:17Z

lib/cadet/assessments/assessments.ex

-    Repo.all(Assessment)
-  end
-
  def all_assessments(assessment_type) do


Do you wanna add the max_xp field here?

indocomsoft · 2018-07-15T15:24:35Z

lib/cadet/assessments/assessments.ex

-    assessment_with_type = Repo.all(from(a in Assessment, where: a.type == ^assessment_type))
-    # TODO: Refactor to be done on SQL instead of in-memory
-    Enum.filter(assessment_with_type, &(&1.is_published and Timex.before?(&1.open_at, now)))
+    Assessment


Same thing w.r.t. max_xp field

i'm thinking of removing this method since assessments_controller is implemented and doesn't use it

Sure, I don't think anyone's gonna miss it.

indocomsoft · 2018-07-15T15:25:47Z

lib/cadet/assessments/assessments.ex

+    Assessment
+    |> where(is_published: true)
+    |> where(type: ^assessment_type)
+    |> where([a], a.open_at <= from_now(1, "second"))


Why not from_now(0, "second")? 😂

idem for the other from_now's

indocomsoft · 2018-07-15T15:28:31Z

lib/cadet/assessments/assessments.ex

+        |> preload([a, q], questions: q)
+        |> Repo.one()
+
+      params_with_assessment_id = Map.put_new(params, :assessment_id, assessment.id)


No check if assessment == nil? Might be better to use with

Or refactor into 2 functions:

def create_question_for_assessment(params, assessment_id) when is_ecto_id(assessment_id) def create_question_for_assessment(params, assessment = %Assessment{})

indocomsoft

Just some small changes needed

indocomsoft · 2018-07-16T13:30:59Z

lib/cadet/assessments/assessments.ex

-    simple_update(
-      Assessment,
-      id,
-      using: &Assessment.changeset/2,


[_, q] lel

indocomsoft · 2018-07-16T13:34:10Z

lib/cadet/assessments/assessments.ex

  def publish_assessment(id) do
-    simple_update(
-      Assessment,
-      id,


|> join(:left, [a], q in assoc(a, :questions))

indocomsoft

LGTM, will merge once Travis passes

* Refactor assessments context to execute single queries where possible * Refactor some assessments tests * Wups * Respond to comments * Remove some unused functions * Fix `create_question_for_assessment` * Minor fixes

tuesmiddt added 3 commits July 8, 2018 19:40

Refactor assessments context to execute single queries where possible

70f1252

Refactor some assessments tests

0e94e7d

Wups

3e2be5d

indocomsoft requested changes Jul 8, 2018

View reviewed changes

Respond to comments

92dec1d

Merge branch 'master' into refactor-assessments-context

7d008f1

Merge branch 'master' into refactor-assessments-context

d73465d

indocomsoft requested changes Jul 15, 2018

View reviewed changes

tuesmiddt and others added 3 commits July 16, 2018 11:32

Remove some unused functions

1daba77

Merge branch 'master' into refactor-assessments-context

90579a4

Fix create_question_for_assessment

b4a2429

indocomsoft requested changes Jul 16, 2018

View reviewed changes

Minor fixes

89c8a09

indocomsoft approved these changes Jul 16, 2018

View reviewed changes

indocomsoft merged commit 42391b9 into master Jul 16, 2018

indocomsoft deleted the refactor-assessments-context branch July 16, 2018 13:52

Refactor assessments context #102

Refactor assessments context #102

Uh oh!

Conversation

tuesmiddt commented Jul 8, 2018 • edited by indocomsoft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 943

💛 - Coveralls

Uh oh!

indocomsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuesmiddt commented Jul 8, 2018

Uh oh!

indocomsoft commented Jul 15, 2018

Uh oh!

indocomsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indocomsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indocomsoft left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

tuesmiddt commented Jul 8, 2018 •

edited by indocomsoft

Loading

coveralls commented Jul 8, 2018 •

edited

Loading

indocomsoft left a comment •

edited

Loading