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.

Sign up for GitHub

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

Jump to bottom

[354] Make animals.sex an enum #371

Merged
merged 9 commits into from Sep 23, 2020
Merged

[354] Make animals.sex an enum #371

merged 9 commits into from Sep 23, 2020

Conversation

viniciusgama
Copy link
Collaborator

Checklist:

  • I have performed a self-review of my own code,
  • I have commented my code, particularly in hard-to-understand areas,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works,
  • New and existing unit tests pass locally with my changes ("bundle exec rake"),
  • Title include "WIP" if work is in progress.

Resolves #354

Description

  • What motivated this change (if not already described in an issue)? Normalize the data round the sex of an animal. Currently this is a open text field which can be difficult to query in the future if different users inputs different values.
  • What alternative solutions did you consider? The current solution which can led to future problems.
  • What are the tradeoffs for your solution? Adds an extra dependency

This also make sure that when inputing the data in the form, the user can only select valid options.

List any dependencies that are required for this change. (gems, js libraries, etc.)

activerecord-postgres_enum

I have chosen this option as it's the one with the most recent releases which indicates it's being actively being developed

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Screenshots

Screen Shot 2020-09-22 at 7 10 50 PM

@viniciusgama
Transform animal sex into an enum
7499577
@viniciusgama
Makes sex an enum field on the animals table
44f9080
@viniciusgama
Merge branch 'master' into 354-transform-sex-into-enum
5631333 
* master:
  use helper method
  Add controller spec, redefine UsersController#admin?
  typo
  use concern instead of standalone logic
  Remove unused UntaggedAnimalAssessment model and related code
  Add organization to tanks and fix related specs
  Set up UsersController#show, include related test examples
  Add organization for families
  Add organizations to families
  Add organization association to animal
@viniciusgama viniciusgama self-assigned this Sep 22, 2020
haydenrou
haydenrou approved these changes Sep 23, 2020
View reviewed changes
app/views/animals/show.html.erb Outdated
@@ -32,7 +32,7 @@

<p>
<strong>Sex:</strong>
<%= @animal.sex %>
<%= @animal.sex&.humanize %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there ever a scenario where there is no sex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure. I've added this here because the field is nullable. Should we restrict this @benreyn @CraigJZ ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually yes! In our meeting with stakeholders yesterday, and in the data files they have sent us, there are many cases where the sex of an animal is unknown or not able to be determined until the animal is more mature.

My preference would be to make the column non-nullable in the database and add a third "unknown" option to the enum.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

implemented here @haydenrou @benreyn 8631e19

@benreyn benreyn added First Project PR First project PR by user and removed First Project PR First project PR by user labels Sep 23, 2020
benreyn
benreyn approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@benreyn benreyn left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks @viniciusgama !

viniciusgama and others added 3 commits September 23, 2020 12:39
@viniciusgama
Merge branch 'master' into 354-transform-sex-into-enum
51719cf
@viniciusgama
Set unknown as the default sex for an animal
959b615
@viniciusgama
Merge branch 'master' into 354-transform-sex-into-enum
d8c8bbb 
* master:
  Remove post settlement inventory model and related code
  using current_organization method to get users
  added show link in index view
  setting up index view for users and adding specs
@viniciusgama viniciusgama merged commit c8ee5bb into rubyforgood:master Sep 23, 2020
@viniciusgama viniciusgama deleted the 354-transform-sex-into-enum branch September 23, 2020 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benreyn benreyn benreyn approved these changes

@haydenrou haydenrou haydenrou approved these changes

@CraigJZ CraigJZ Awaiting requested review from CraigJZ

Assignees

@viniciusgama viniciusgama

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make animals.sex an enum
3 participants
@viniciusgama @benreyn @haydenrou