Skip to content

Conversation

@begedin
Copy link
Contributor

@begedin begedin commented Mar 20, 2017

What's in this PR?

This issue has been a common pattern recently, so this PR is an attempt to alleviate it.

In this specific case, adding the constraint that a project's long description needs to be present for the project to be approved broke the seeds script. I fixed that, but then I also made an attempt to make the seeds as simple as possible, as well as more standards compliant.

Changes

  • Where ever possible, the seed data is now explicit. We do not rely on changesets and instead define data directly using schema structs. The only exception for this is the User, for which we need the registration changeset in order to encrypt the password. However, even that can be alleviated in a separate PR if necessary.
  • I've replaced the usage of the cond clause with a case clause. I believe even credo will warn us against using cond in cases where there are less than 3 conditions, one of which is true. Those cases are made for a case statement. It also makes it so that the primary condition is not a negative ([] instead of ![]) which is easier to read.
  • Eliminated the double access for the user seeds by using Ecto.Changeset.merge (a negligible speed-up, but technically the more correct way to do it.

Advantages

  • volunteers will have an easier time updating the seeds. Thee seeds are no longer coupled to the rest of our code, and everything is in one place, allowing them to have an easier grasp of what needs to be updated.
  • additionally, it will allow us to have an easier grasp of what is being updated and review accordingly

Disadvantages

  • Some duplication. We explicitly create task lists, instead of using the predefined function on the TaskList module). I believe this is still worth it. That definition will rarely be changed.
  • If there's a changeset-level validation which is not supported by the database itself (no database-level constraint for it), then a seed might insert something that the application considers invalid. I personally believe this should be treated as a bug in the vast majority of cases anyway.

@begedin
Copy link
Contributor Author

begedin commented Mar 20, 2017

This is good for review. I've also created #774 to further alleviate this issue.

@joshsmith
Copy link
Contributor

Agree with everything here, and think the disadvantages were well-considered, including the need to have database-level constraints where they do not exist. I think there are instances where this is the case, and that should be alleviated some here.

@joshsmith joshsmith merged commit 7521582 into develop Mar 20, 2017
@joshsmith joshsmith deleted the fix-and-simplify-seeds branch March 20, 2017 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants