-
Notifications
You must be signed in to change notification settings - Fork 123
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
Create AdopterFosterAccount #551
Conversation
Please review and let me know if this approach seems appropriate. This adjustment involved only altering the model name, so I updated the wording in the model files accordingly while keeping the relationships unchanged. Planned Steps:
Let me know your thoughts. |
db/migrate/20240315160350_rename_adopter_accounts_to_adopter_foster_accounts.rb
Outdated
Show resolved
Hide resolved
Yes your steps seem appropriate. I have left a series of comments. One thing, while you have started a new branch, it looks like you have not reset your database and you have committed schema changes from the old PR where we were going to use polymorphic association. Note that your database, unless you create a new one for a separate branch, or recreate it entirely, will still have the schema from whatever migrations you have run on it. In this case, it has the changes from the migrations in the old PR. So please drop and recreate your db, paying a mind to which migrations you have in this branch, and ensure you only have the migrations from main branch + whatever you need for this PR. That will re-write the schema and remove the unwanted changes we have above. Thanks! |
4995264
to
e5d791e
Compare
@kasugaijin I hope I've left this ticket in a decent spot for you. The schema should now reflect the correct changes. The next steps involve updating the seed file and modifying all instances of the adopter_accounts. |
…ce to adopter_foster_account id and change references in code to adopter account to use the new model/method name
Thanks @ErinClaudio! You did most of the work. Just two quick commits after. |
Great job! Would it be possible to combine all the migrations into one migration file? I think we can also rename columns which would help retain data |
Done. I split the table rename and column renames into two migration files. I also added foreign key constraints for the belong_to relationships. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! I think we should update the authorization permission names to reflect the AdopterProfile
to AdopterFosterProfile
name change, but I can do that in a separate PR once this is merged.
Thank you for all the work, Erin!
Good point! This branch needs main merging in and then running find and replace again. I can do that. |
…er account and remove unnecessary changes in migration file.
@mononoken I made changes to policy permission names (commit below) and added unique constraint to the index as suggested. FYI I am finishing this off for Erin as he is away for 10 days - @ErinClaudio is aware I was taking over :) |
Thanks for taking over, @kasugaijin ! The new changes look good. |
@mononoken feel free to approve and merge if you want to! |
Nice! This unblocks a lot of work! |
🔗 Issue
#449
✍️ Description
📷 Screenshots/Demos