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

Migration for foorm library model #38681

Merged
merged 3 commits into from Jan 23, 2021
Merged

Migration for foorm library model #38681

merged 3 commits into from Jan 23, 2021

Conversation

bencodeorg
Copy link
Contributor

@bencodeorg bencodeorg commented Jan 22, 2021

Creates a Library model for foorm, representing a collection of LibraryQuestions. This entity should make it easier to reason about saving changes to library questions, which will require saving the entire library to which the question belongs. See #38682 for an immediate follow-up putting this to initial use.

Testing

I executed the migration locally via bundle exec rake db:migrate.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@cforkish cforkish left a comment

Choose a reason for hiding this comment

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

LGTM! one question about 'published', but no blockers.

create_table :foorm_libraries do |t|
t.string :name, null: false
t.integer :version, null: false
t.boolean :published, default: true, null: false
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about using true as the default here. i would expect libraries to start out unpuplished, no? imo might make more sense to set this to true during seeding so that once we support creating new libraries (if that's ever going to be a thing), new libraries will start out unpubplished. but maybe i don't have proper context on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can give context on this. When we started adding the publish option for forms/library questions I set the default to true because none of the forms had a pre-set published state yet, and we aren't using the published state currently. I think it could be worth changing the default in the future when we have a more formalized use of published.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good call -- looks like the seeding logic defaults to true, so we're already doing what you suggested :). I think I'll just skip having a db-level default, and rely on places where we save libraries (currently, only during seeding) to decide whether it should be default published or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@molly-moen do you think it's reasonable to not have a DB-level default since the seeding logic sets published to true if it's missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

my concern was that is while we will almost always create libraries by seeding, I didn't want us to get into a case where the published state was nil if for some reason that process changes. It is an edge case though, and not a major issue if it does end up nil, so I will leave it up to you.

Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

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

Looks good! I wonder if we should add a column to library questions for library id. We are referencing everything by name and version (and we do queries on library questions by name and version), but for rails-y queries it might make things easier to also have id? I don't have a strongly held opinion on this but am throwing it out there :)

@bencodeorg
Copy link
Contributor Author

Looks good! I wonder if we should add a column to library questions for library id. We are referencing everything by name and version (and we do queries on library questions by name and version), but for rails-y queries it might make things easier to also have id? I don't have a strongly held opinion on this but am throwing it out there :)

Yeah good point...it does make stuff like this more verbose. I think I'll leave as-is for now but can revisit if we'd like later.

@bencodeorg bencodeorg merged commit 2d15fdb into staging Jan 23, 2021
@bencodeorg bencodeorg deleted the ben/foorm-library-model branch January 23, 2021 01:11
@davidsbailey
Copy link
Member

Hey @bencodeorg , I am seeing some diffs after running this migration:

Screen Shot 2021-01-25 at 9 46 42 AM

It appears to be because there were no updates to schema.rb or foorm/library.rb in your last commit. Would you be willing to take a crack at fixing this?

@bencodeorg
Copy link
Contributor Author

Hey @bencodeorg , I am seeing some diffs after running this migration:

Screen Shot 2021-01-25 at 9 46 42 AM

It appears to be because there were no updates to schema.rb or foorm/library.rb in your last commit. Would you be willing to take a crack at fixing this?

Ugh I always mess something up with migrations. Will fix, thanks for the heads up!

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.

None yet

4 participants