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

Week number to events and eras #81

Merged
merged 5 commits into from
Jan 6, 2021
Merged

Conversation

LHJE
Copy link
Collaborator

@LHJE LHJE commented Jan 5, 2021

Code Highlights:

  • Simply adds week_number to Eras and Events migrations, and subsequently the schema

Where should the reviewer start?

  • The Schema is probably a good one!

Have Tests Been Added?

  • No.
  • Yes, but all tests are not passing.
  • Yes, and all are passing.

Any background context you want to provide?

Message/Questions for reviewer:

Issues:

Screenshots (if appropriate):

Tracking Consistency:

  • added appropriate labels
  • My code follows the code style of this project and has removed all unnecessary annotations
  • I have added comments on my pull request, particularly in hard-to-understand areas
  • looked at PR preview to check spelling, syntax, formatting, and completion
  • Rubocop Violations:

@LHJE LHJE added this to the Life Eras milestone Jan 5, 2021
@LHJE LHJE self-assigned this Jan 5, 2021
@leahriffell
Copy link
Collaborator

Thanks, Luke! Should we also assert presence of week_num in the events and eras models+specs?

@LHJE LHJE mentioned this pull request Jan 5, 2021
8 tasks
Copy link
Collaborator

@AngelaGuardia AngelaGuardia left a comment

Choose a reason for hiding this comment

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

Thanks Luke! I agree with @leahriffell. Let's add that week presence validation to the models and this will be ready to merge.

Copy link
Collaborator

@AngelaGuardia AngelaGuardia left a comment

Choose a reason for hiding this comment

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

Minor change!

@@ -5,6 +5,7 @@ def change
t.string :name
t.date :start_date
t.date :end_date
t.integer :week_number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Eras should have a start_week and end_week

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got iiiit! Okay cool! I'll have that working today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be good to go!

@leahriffell
Copy link
Collaborator

I tried adding the model tests to your branch (validate week num presence on eras/events) but it actually broke all of the tests and factories that didn't have these. I see in your later PR you add these in so I think we should add the model specs to that PR. I'll merge this one and then we can add model specs to that one!

@leahriffell leahriffell merged commit 9059a12 into main Jan 6, 2021
@leahriffell leahriffell deleted the week_number_to_events_and_eras branch January 6, 2021 17:07
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.

Add week number to events and eras table
3 participants