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

Her 17 migration for renaming kit requests to orders #12

Merged
merged 11 commits into from
Dec 5, 2024

Conversation

olesiamironenko
Copy link
Collaborator

Issue

https://raquelanaroman.atlassian.net/browse/<JIRA_TICKET_ID>

Create a migration that renames the kit_requests table to orders

Changes

  1. I generated the migratiion for renaming the table kit_requests to orders.
  2. I edited the code in the migration file by adding "rename_table :kit_requests, :orders" to the "change" method.
  3. I run the migration.
  4. I staged, committed, and pushed changes to the github repository.

Review Checklist

  • I have documented my code with code comments.

@raquii
Copy link
Collaborator

raquii commented Nov 22, 2024

Similar to my comments to @mhope21 in #9, you would normally need to fix the failing tests before this PR can be approved.

However, this is a subtask ticket and we are renaming the entire model, I am going to ask you to complete the other subtask, https://raquelanaroman.atlassian.net/browse/HER-18, and complete it on this same branch.

Otherwise, we will break tests for everyone if we merge this "as is", (which is why merging isn't allowed when tests fail).

The migration looks great, though! ✅

mhope21
mhope21 previously approved these changes Nov 25, 2024
Copy link
Collaborator

@mhope21 mhope21 left a comment

Choose a reason for hiding this comment

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

Looks great. Very thorough. We are encouraged to comment our changes I think, but this is really good. I think there are some conflicts to resolve on your PR. Check it out.

@olesiamironenko
Copy link
Collaborator Author

Looks great. Very thorough. We are encouraged to comment our changes I think, but this is really good. I think there are some conflicts to resolve on your PR. Check it out.

Thanks, Marcia!

dewi-anggraini
dewi-anggraini previously approved these changes Nov 26, 2024
Copy link
Collaborator

@dewi-anggraini dewi-anggraini left a comment

Choose a reason for hiding this comment

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

This looks great.

Copy link
Collaborator

@raquii raquii left a comment

Choose a reason for hiding this comment

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

Good. Just one thing: I think we missed replacing kit_requests here:

has_many :kit_requests

@olesiamironenko
Copy link
Collaborator Author

Good. Just one thing: I think we missed replacing kit_requests here:

has_many :kit_requests

Since there are those lines in order model:
belongs_to :kit
belongs_to :user

and in user model:
has_many :orders

should it be this line in kit model?
has_many :orders

@raquii raquii requested a review from mhope21 December 5, 2024 02:07
Copy link
Collaborator

@mhope21 mhope21 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.

@raquii raquii merged commit e5ea3dc into main Dec 5, 2024
3 checks passed
@raquii raquii deleted the HER-17-Migration-for-renaming-kit-requests-to-orders branch December 5, 2024 02:14
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.

4 participants