Skip to content
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

Remove TaskSkill reliance on ja_resource/canary #964

Conversation

landongrindheim
Copy link
Contributor

@landongrindheim landongrindheim commented Sep 23, 2017

Removes reliance and ja_resources and canary for the TaskSkillController

References

Closes #912

Progress on: #864

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor change in the tests is needed, but otherwise looks great.

Don't forget to rebase and squash the commits as you make the change, for quick approval and merging from my side.

changeset = %TaskSkill{} |> create_changeset(%{task_id: task.id})
refute create?(user, changeset)
task_skill = %TaskSkill{task_id: task.id}
refute create?(user, task_skill)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In "real-life", this function is being called from CodeCorps.Policy using a params map (with string keys) as the second argument.

However, the tests call it with a %TaskSkill{} with atom keys as the second argument.

The real life part is indirectly tested from the controller tests, and the tests you wrote here still pass, becase in CodeCorps.Policy.Helpers, we have

def get_task(%TaskSkill{task_id: task_id}), do: Repo.get(Task, task_id)
def get_task(%{"task_id" => task_id}), do: Repo.get(Task, task_id)

However, these tests leave room for a potential bug being introduced, so I would replace the second argument in create? tests with a params map. Note that the delete? tests do correctly use a %TaskSkill{} as the second argument, so there isn't anything to change there.

@landongrindheim landongrindheim force-pushed the 912-remove-outdated-dependencies-from-task-skill-controller branch from 58cfd2c to c3093b6 Compare September 25, 2017 12:41
@landongrindheim
Copy link
Contributor Author

@begedin Thanks for the feedback! The branch has been updated :)

@begedin begedin merged commit 0342c8c into code-corps:develop Sep 25, 2017
@begedin
Copy link
Contributor

begedin commented Sep 25, 2017

@landongrindheim Thank you. Awesome work!

@joshsmith
Copy link
Contributor

🙌 , as always. 🥇

@landongrindheim landongrindheim deleted the 912-remove-outdated-dependencies-from-task-skill-controller branch October 10, 2017 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants