Add pull_requests && done to task_list #1157

snewcomer · 2017-11-06T05:49:06Z

done boolean
pull_requests boolean
script for modifying task_lists and project tasks
unique constraints for pairs of (project_id, inbox), (project_id, pull_requests), (project_id, done)

joshsmith · 2017-11-06T05:55:13Z

lib/code_corps/model/task_list.ex

@@ -52,7 +56,7 @@ defmodule CodeCorps.TaskList do

  def create_changeset(struct, params) do
    struct
-    |> cast(params, [:inbox])
+    |> cast(params, [:inbox, :done, :pull_requests])


Would alpha-order here.

joshsmith · 2017-11-06T05:57:19Z

priv/repo/modify_task_list_done.exs

+    Task
+    |> CodeCorps.Helpers.Query.project_filter(%{project_id: project.id})
+    |> Repo.all()
+    |> Enum.each(&assign_task_to_done(&1, Enum.at(project.task_lists, 3)))


What's this doing?

Calls assign_task_to_done(&1, 3rd_task_list_in_project), which is, I assume, the task list with done: true. It would, however, be preferable to call update_tasks_to_done/1 with that task list as an argument.

joshsmith · 2017-11-06T05:58:35Z

lib/code_corps/model/task_list.ex

        name: "In Progress",
        position: 3
      }, %{
        inbox: false,
+        done: true,
        name: "Done",
        position: 4
      }


I would probably remove inbox: false from the others here for consistency.

@joshsmith remove all inbox: false?

joshsmith · 2017-11-06T06:01:04Z

priv/repo/modify_task_list_done.exs

+    TaskList.create_changeset(task_list, %{done: true})
+    |> Repo.update()
+  end
+  defp update_done_column(task_list), do: task_list


This is good, but will also need a similar fn for %TaskList{name: "In Progress"} to make it the pull_requests list.

joshsmith · 2017-11-06T06:15:23Z

priv/repo/modify_task_list_done.exs

+    TaskList
+    |> Repo.all()
+    |> Enum.each(&update_pr_column/1)
+


This could be condensed down to:

TaskList |> Repo.all() |> Enum.each(&update_done_column/1) |> Enum.each(&update_pr_column/1)

begedin · 2017-11-06T10:30:46Z

@snewcomer @joshsmith

I've used the migration script you wrote, Scott, and moved it's behavior into the migrations themselves. I did not get a chance to comment on the issues this PR resolves, so the instructions there do not match what I did, but I really feel this should be our preferable approach to handling migrations.

The code that migrates the data uses schema names instead of our schema modules, so it's agnostic to our actual modules and works regardless of what the rest of our code looks like.

Due to separate up and down steps, we are also able to rollback and I was able to test it using semi-real data (by syncing a mock repo and trying it out on that).

begedin

@snewcomer Great work. I made some changes to how migrations are done, worded a couple of your tests differently and cleaned up.

This is about done, so I'm approving your part from my end. @joshsmith can review and approve my part.

begedin · 2017-11-06T11:26:41Z

test/lib/code_corps/model/task_list_test.exs

+  test "defaults :pull_requests to 'false'" do
+    {:ok, record} =
+      %TaskList{} |> TaskList.changeset(@valid_attrs) |> Repo.insert
+    assert record.pull_requests == false


@snewcomer Your assertion here was refute record.pull_requests, but that one would have passed even if record.pull_requests == nil, so we want a more explicit one.

🤦‍♂️

begedin

Just adding a blank "request changes" review here, so we don't accidentally merge before @joshsmith reviews.

begedin · 2017-11-06T11:32:37Z

priv/repo/migrations/20171106050209_add_pull_requests_to_task_list.exs

+          where: not is_nil(gi.github_pull_request_id),
+        update: [set: [task_list_id: ^pr_list_id]]
+      ) |> Repo.update_all([])
+    end)


@joshsmith I really feel this is the way to go with migrations.

If our schema modules do not match the schemas in any migration step, the code will fail, but if we go like this, "schemaless", the only case where the migration step would not match the actual state of the schema were if we somehow run it manually, out of order, which would be incorrect usage of the migration.

I understand using actual schema modules and changesets in a seed script, but they should not be used in migrations.

I really like this.

Unfortunately it looks like this will override some of the work that was done in the earlier migration that moved tasks to closed, because this is not checking for whether the issues are closed.

@joshsmith You are correct about checking for gi not closed. You could also check for task not closed, it would likely serve the same purpose, since presumably, the task is otherwise in sync with the issue.

On a similar note, my other migration should probably add a clause for archived. I'm not sure what the exact query would be, but it would likely involve a where: date_add(t.modified_at, 30, "day") > ^Date.utc_today somewhere in it.

I can't push the change myself due to not being home, but that should help.

snewcomer

👍

snewcomer · 2017-11-06T14:40:24Z

test/lib/code_corps/model/task_list_test.exs

+  test "defaults :pull_requests to 'false'" do
+    {:ok, record} =
+      %TaskList{} |> TaskList.changeset(@valid_attrs) |> Repo.insert
+    assert record.pull_requests == false


🤦‍♂️

snewcomer · 2017-11-06T14:42:50Z

test/lib/code_corps/model/task_list_test.exs

+        |> Repo.insert
+
+      refute changeset.valid?
+        assert changeset |> Map.get(:errors) |> Keyword.get(:done)


minor spacing...unless its the GUI.

snewcomer · 2017-11-06T14:44:07Z

priv/repo/migrations/20171106103153_add_unique_constraints_to_specific_task_lists.exs

+
+    create unique_index(
+      "task_lists", [:project_id],
+      where: "done = true", name: "task_lists_project_id_done_index")


Nice work!!

snewcomer · 2017-11-06T14:48:55Z

priv/repo/migrations/20171106050209_add_pull_requests_to_task_list.exs

+          where: not is_nil(gi.github_pull_request_id),
+        update: [set: [task_list_id: ^pr_list_id]]
+      ) |> Repo.update_all([])
+    end)


I really like this.

joshsmith · 2017-11-06T18:14:47Z

priv/repo/migrations/20171106050209_add_pull_requests_to_task_list.exs

+          where: not is_nil(gi.github_pull_request_id),
+        update: [set: [task_list_id: ^pr_list_id]]
+      ) |> Repo.update_all([])
+    end)


Unfortunately it looks like this will override some of the work that was done in the earlier migration that moved tasks to closed, because this is not checking for whether the issues are closed.

Fixed

snewcomer added Difficulty: Medium needs code review Priority: High labels Nov 6, 2017

snewcomer self-assigned this Nov 6, 2017

snewcomer requested review from joshsmith and begedin November 6, 2017 05:49

joshsmith reviewed Nov 6, 2017

View reviewed changes

begedin force-pushed the task-list-modify branch 2 times, most recently from da90d36 to 3a48282 Compare November 6, 2017 11:20

begedin approved these changes Nov 6, 2017

View reviewed changes

begedin previously requested changes Nov 6, 2017

View reviewed changes

begedin force-pushed the task-list-modify branch from c714aa5 to 03a06f0 Compare November 6, 2017 11:30

begedin reviewed Nov 6, 2017

View reviewed changes

snewcomer commented Nov 6, 2017

View reviewed changes

begedin mentioned this pull request Nov 6, 2017

Assign task lists and archived during sync #1158

Merged

joshsmith requested changes Nov 6, 2017

View reviewed changes

Add pull_requests and done columns to task_list with migrations

994f7f3

joshsmith force-pushed the task-list-modify branch from dff22be to 994f7f3 Compare November 6, 2017 19:24

joshsmith approved these changes Nov 6, 2017

View reviewed changes

joshsmith added ready to merge and removed needs code review labels Nov 6, 2017

joshsmith merged commit 1ba8911 into develop Nov 6, 2017

joshsmith deleted the task-list-modify branch November 6, 2017 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pull_requests && done to task_list #1157

Add pull_requests && done to task_list #1157

snewcomer commented Nov 6, 2017 •

edited

joshsmith Nov 6, 2017

joshsmith Nov 6, 2017

begedin Nov 6, 2017

joshsmith Nov 6, 2017

snewcomer Nov 6, 2017

joshsmith Nov 6, 2017

joshsmith Nov 6, 2017

begedin commented Nov 6, 2017

begedin left a comment

begedin Nov 6, 2017

snewcomer Nov 6, 2017

begedin left a comment

begedin Nov 6, 2017 •

edited

snewcomer Nov 6, 2017

joshsmith Nov 6, 2017

begedin Nov 6, 2017

snewcomer left a comment

snewcomer Nov 6, 2017

snewcomer Nov 6, 2017

snewcomer Nov 6, 2017

snewcomer Nov 6, 2017

joshsmith Nov 6, 2017

Add pull_requests && done to task_list #1157

Add pull_requests && done to task_list #1157

Conversation

snewcomer commented Nov 6, 2017 • edited

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

begedin commented Nov 6, 2017

begedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin left a comment

Choose a reason for hiding this comment

begedin Nov 6, 2017 • edited

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

snewcomer 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

snewcomer commented Nov 6, 2017 •

edited

begedin Nov 6, 2017 •

edited