Implement limit and scope variable in same causes query #126

aonomike · 2019-11-13T20:28:59Z

What does this PR do?

Enables the causes query to use both the scope and limit variables together

How should this be manually tested?

On terminal, run command ...
Play around with this query and variables on you graphiql play ground

query($limit: Int, $scope: String){
  causes(limit: $limit, scope: $scope) {
    id
    name
    amountRaised
    endDate
    artists{
      name
    }
  }
}

variables

{"scope": "ending_soon", "limit": 3}

What are the relevant Github Issues ?

Fixes #123

- Implements that the scope and limit variables are used to manipulate the result of the causes query Co-Authored-By: Azure May <azure.may.burmeister@gmail.com>

- Add ability for trending causes to be limited in the causes query Co-Authored-By: Azure May <azure.may.burmeister@gmail.com>

yakryder

Overall looks good @aonomike. I do have to confess not knowing how well-formed ecto queries should look in a well-organized codebase in the wild, so I'm [perhaps wrongly] generalizing based on principles and opinion. Lots of thoughts.

The core ideas of the queries you're adding don't have explicit contracts. There is no cutoff criteria for trending -- trending is just the newest causes making the most money. If the system had no causes that had taken in money, the newest one would be trending.

The same is true of no explicit contract for ending soon. I've left a more detailed note on that one.

yakryder · 2019-11-14T20:22:58Z

test/sing_for_needs_web/schema/query/causes_test.exs

+    five_days_ago = Timex.add(Timex.now(), Timex.Duration.from_days(-5))
+    nine_days_ago = Timex.add(Timex.now(), Timex.Duration.from_days(-9))
+
+    insert(:cause, %{name: "Cause ends in 30 days", end_date: thirty_days_from_now})


If we haven't started this process already, we should start thinking about extracting some of these low-level test data creation details into separate modules. Doesn't need to go in with this PR -- and arguably shouldn't if this kind of organization doesn't yet exist in the project.

Agree, let me refactor these to a helper file...

It's also fine to leave it and circle back for it in a dedicated refactor.

But since factories already exist in the project, I think you can justify moving those Causes used by these tests in this PR.

yakryder · 2019-11-14T20:25:56Z

lib/sing_for_needs/causes.ex

@@ -43,6 +43,28 @@ defmodule SingForNeeds.Causes do
    |> Repo.all()
  end

+  defp causes_query(%{scope: scope, limit: limit}) do
+    case scope do
+      "trending" ->


You're hard-coding the notion of a cause trending to a specific implementation here. I wouldn't recommend that. It's unlikely that "amount raised" will stay as the definer of a a cause's trending-ness.

yakryder · 2019-11-14T20:30:05Z

lib/sing_for_needs/causes.ex

+    case scope do
+      "trending" ->
+        from(c in Cause,
+          left_join: a in assoc(c, :artists),


I'm not sure I would try and hold this up for it, but personally I don't approve of one-letter variable names anywhere except an occasional raw SQL query.

I did check and see that the Ecto documentation favors this style. I think that's unfortunate, but it does set an ecosystem precedent that one could choose to follow.

Thanks @Sherspock I think the one letter aliases is synonymous with the way sql queries handles aliases on queries. I think ecto just borrows from them :)

Yep. I don't think it should, but I can't harp on it if it's ecosystem and codebase compliant

yakryder · 2019-11-14T20:32:24Z

lib/sing_for_needs/causes.ex

@@ -43,6 +43,28 @@ defmodule SingForNeeds.Causes do
    |> Repo.all()
  end

+  defp causes_query(%{scope: scope, limit: limit}) do


can't help but think that multiple function heads would be more idiomatic than a case statement here

When I use multiple functions with pattern matching I keep getting this strange

** (Postgrex.Error) ERROR 22001 (string_data_right_truncation) value too long for type character varying(255)

maybe I can figure this out on a differerent PR

Interesting. This should be from overflowing a varchar(255) column. I don't have any immediate thoughts as to why you'd be seeing it here.

Yeah...actually was caused by faker.

yakryder · 2019-11-14T20:39:31Z

lib/sing_for_needs/causes.ex

+          order_by: [desc: :amount_raised, desc: count(a.id)],
+          group_by: c.id,
+          limit: ^limit,
+          select: c


As long as it looks at least as readable (if not more so) when you look at it -- and depending on the override behavior -- I would think about extracting the common elements of these queries into a base cause query and then compose from there.

yakryder · 2019-11-14T20:48:53Z

test/sing_for_needs_web/schema/query/causes_test.exs

+      }
+    """
+
+    artists = insert_list(4, :artist)


OK I see we've got factories. May want to thinking about making some cause factories

Yes we are already using Factories for Causes and Artists, we may just need to have them definitions done in a helper file.

yakryder · 2019-11-14T20:53:48Z

lib/sing_for_needs/causes.ex

+      "ending_soon" ->
+        from(c in Cause,
+          where: c.end_date > ^Timex.now(),
+          order_by: [asc: c.end_date],


The notion of "ending soon" here isn't really what it says it is -- this query actually is "in the future, sorted by end date". That leaves the notion of "ending soon" with no concrete contract -- if the system had no causes ending until two months from now, "soon" would be two months, then the end date of the next record, etc.

Thanks @Sherspock I guess these criteria has not been clearly defined. So I was just looking for the first two causes with the closest ending date...maybe the meaning of the word soon may be different, but this still needs to be defined.

My suggestion is define it better. This is the only part of the PR I'm seriously considering insisting on changes for.

It seems to me there should be an actual contract for "soon" -- i.e the query pulls back causes ending within a specific window of time. If that seems undesirable to you, the name should be changed to reflect the results that the query does pull back.

so we can fix it to 3 months? @are you in agreement @Sherspock @javpet

3 months seems very long to me based on implicit [and very fallible] assumptions that the average duration of a cause will be shorter than three months and that for a cause to be "ending soon" it should be at least 50% (and probably 75% or more) complete.

But it does do the job of firming up the contract.

I'll defer to @javpet

yakryder · 2019-11-14T21:01:20Z

test/sing_for_needs_web/schema/query/causes_test.exs

+
+    conn = build_conn()
+
+    conn =


See note below about [my fallible opinions on] local variable naming, an extremely helpful thing for the test reader and extremely inexpensive thing to change

conn_after_trending_causes_query? Little wordy, but something to clarify what it is and avoid mutating 253's conn, which it can be argued in Elixir feels a little unidiomatic.

I think phoenix conventionally would want us to call the conn object as just conn, as I have seen in many projects I tend to borrow from. maybe a little confusing if named different as other people may not get to know its a conn object just by the first look? what do you think?

For the first conn by all means. For the second conn, conn_after_trending_causes_query makes the state of that conn struct a lot clearer (albeit verbosely). Just naming it conn doesn't provide the same kind of help to a test reader.

I'm not saying you should definitely change it -- I'm bringing to your attention something that I would probably do.

yakryder · 2019-11-14T21:01:27Z

test/sing_for_needs_web/schema/query/causes_test.exs

+      }
+    }
+
+    assert expected_result == json_response(conn, 200)


Personally I don't mind the additional overhead of an actual_result (or more specifically named) variable as the subject of comparison here.

yakryder · 2019-11-14T21:06:05Z

lib/sing_for_needs/causes.ex

@@ -43,6 +43,28 @@ defmodule SingForNeeds.Causes do
    |> Repo.all()
  end

+  defp causes_query(%{scope: scope, limit: limit}) do


You're not allowing the injection of any other query params here -- that's more of an observation than anything else, you don't seem to need them now.

yakryder · 2019-11-14T21:12:15Z

@aonomike Have left some thoughts. Please look over at your leisure. Don't know that any of them are blockers

aonomike · 2019-11-15T08:38:21Z

Wow! Thanks @Sherspock for your thorough review, I must admit I have missed them in the recent past...I will have a look at each of them and respond. Thanks!

yakryder

Hey Mike,

This query implementation is very different from the one you did for Artists and it doesn't seem like idiomatic Phoenix. If it were just unidiomatic I'd say let's go ahead and get it in as long as it does what it's supposed to and the tests pass.

But it's also doing some weird things that undermine a key feature of Ecto queries, their composability:

It takes a map.
Drawbacks:
If you want to just pass only limit or only scope as options, the function won't match.
As this doesn't accept an Ecto.Query as its first argument, you can't chain it after another query-building function call.
It doesn't return an Ecto.Query.
Drawback:
As this doesn't return an Ecto.Query, you can't chain it before another query-building function call.

I'm confident a glance at the following articles/screencast will help:
https://blog.drewolson.org/composable-queries-ecto
https://elixirschool.com/blog/ecto-query-composition/
https://elixircasts.io/composing-ecto-queries

It's not essential that the query implementations for Artists and Causes end up following the same underlying pattern with this PR. They do need to ultimately. But we don't want to introduce a change that is both unidiomatic and diverges significantly and without justification from the implementation of another queryable entity.

If you do think the current implementation is idiomatic or that the divergence in implementation is justified I'd be happy to hear why. I'll be the first to admit I'm not a Phoenix insider.

- In test_helpers.exs - Remove redundant setup code for causes

aonomike · 2019-11-15T21:05:26Z

hi @Sherspock thanks for the great explanation, one thing that maybe is not clear to me...by idiomatic do you mean the use of the pipe operator to compose the query? I will go through the links shared for better understanding and get back to you :)

yakryder · 2019-11-15T22:13:59Z

hi @Sherspock thanks for the great explanation, one thing that maybe is not clear to me...by idiomatic do you mean the use of the pipe operator to compose the query? I will go through the links shared for better understanding and get back to you :)

Based on my fallible understanding of conventions around the syntax for Ecto queries (which might not be as clear as yours), there isn't an ecosystem consensus around keyword vs. expression syntax for queries. I prefer dots for LINQ and pipes for Ecto, but those are just my opinions. I do think we should try to use the same syntax throughout the codebase, but that's a more opinionated stance than many developers.

By not idiomatic I mean I don't think this is what Ecto queries are supposed to look like. In addition to the drawback of not accepting only a scope or only a limit argument -- arguably more a question of a klunky interface than not being Phoenix-y -- perhaps the biggest reason is that you can't compose queries with causes_query/1 using out-of-the-box Ecto functionality.

I could of course be wrong.

yakryder · 2019-11-15T22:20:24Z

I'd be happy to pair with you on this this weekend if we can find a time that works for both of us. Feel free to reach out on Slack if that sounds like something that'd interest you

… now

yakryder

Good job Mike. The querying approach is a definite improvement.

The changes to the tests are more ambiguous. Case in point, it's no longer possible for a reader to understand what's happening in the test cases without traveling to another file. Extracting the causes into well-named factory types called from within the relevant test cases -- or waiting to tackle this issue in another PR -- would have avoided this problem.

The names in the causes array of the expected and actual responses have also become more generic, meaning that a reader of a test failure will have to work harder to understand what went wrong.

I think I understand why you took this approach -- it's DRYer. But the tests have become less clear rather than more clear as a result. They also arguably have not become any more maintainable, and a compelling argument could be made that they have become less so. These two things point to a problem with the abstraction approach. Shielding a reader from lower-level details should help rather than hinder comprehension, and should make code easier to use and change.

Initially I asked for changes because the query approach for Causes was both diverging from Artists and doing so in a way that didn't feel Phoenix-y. While there is still room for future refinement you've done a great job fixing that problem.

The tests, on the other hand, have been made less understandable by the setup extraction but even more so by the combining of details needed for "ending soon" testing with details needed for "trending" testing on the sample Causes. I would recommend reverting these changes but I won't hold up the merge for it.

Thanks!

aonomike · 2019-11-18T04:08:48Z

Thanks @Sherspock . Do you suggest we keep the setup functions on the same file? so we can have setup_trending_causes and setup_causes_ending_soon? Sorry I think I misunderstood your earlier suggestion of setting up the tests from outside the test cases. I think the current approach is less noisy but requires more hard work to understand. I will revert the changes and look at a better way to make the test cases less noisy as well. Thanks!

javpet · 2019-11-24T17:03:37Z

Thanks @Sherspock for looking at this PR and shared your knowledge! I have 0 knowledge on Phoenix or Elixir so technically we're quite dependent on anything we find online.

aonomike and others added 2 commits November 13, 2019 23:02

Implement limit and scope together

eb5b6b0

- Implements that the scope and limit variables are used to manipulate the result of the causes query Co-Authored-By: Azure May <azure.may.burmeister@gmail.com>

Implement limit on trending causes

4de431d

- Add ability for trending causes to be limited in the causes query Co-Authored-By: Azure May <azure.may.burmeister@gmail.com>

aonomike requested review from javpet and yakryder November 13, 2019 20:48

yakryder reviewed Nov 14, 2019

View reviewed changes

yakryder requested changes Nov 15, 2019

View reviewed changes

aonomike added 3 commits November 15, 2019 23:27

Refactor to set up Causes

8daa24b

- In test_helpers.exs - Remove redundant setup code for causes

Refactor to simplify the assert

d569f7a

Refactor to remove case statement for better readability

ac087c8

aonomike added 4 commits November 16, 2019 21:49

Refactor to have query composed

96138d3

Make failing test pass

7a80160

Fix causes ending soon to causes with end date less than 90 days from…

4024213

… now

Refactor the naming of the conn object

413a55b

yakryder approved these changes Nov 18, 2019

View reviewed changes

javpet merged commit aac93a3 into AgileVentures:develop Nov 24, 2019

Implement limit and scope variable in same causes query #126

Implement limit and scope variable in same causes query #126

Conversation

aonomike commented Nov 13, 2019 • edited

What does this PR do?

How should this be manually tested?

What are the relevant Github Issues ?

yakryder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakryder commented Nov 14, 2019

aonomike commented Nov 15, 2019

yakryder left a comment

Choose a reason for hiding this comment

aonomike commented Nov 15, 2019

yakryder commented Nov 15, 2019 • edited

yakryder commented Nov 15, 2019

yakryder left a comment

Choose a reason for hiding this comment

aonomike commented Nov 18, 2019

javpet commented Nov 24, 2019

aonomike commented Nov 13, 2019 •

edited

yakryder commented Nov 15, 2019 •

edited