search

rubyforgood / pet-rescue

Pet Rescue is an application making it easy to link adopters/fosters with pets. We work with grassroots pet rescue organizations to understand how we can make the most impact.
MIT License
57 stars 95 forks source link

819 replace page text with custom page #854

Closed Trevor-Robinson closed 1 month ago

Trevor-Robinson commented 1 month ago

🔗 Issue

https://github.com/rubyforgood/pet-rescue/issues/819

✍️ Description

Changed all occurrences of page text to custom page. Created migration to update table name as well. All tests are passing with new name.

jmilljr24 commented 1 month ago

@all-contributors please add @Trevor-Robinson for code.

allcontributors[bot] commented 1 month ago

@jmilljr24

I've put up a pull request to add @Trevor-Robinson! :tada:

jmilljr24 commented 1 month ago

@Trevor-Robinson Thanks for working on this! Can you take a look at the conflicts when you get a chance?

kasugaijin commented 1 month ago

hey @Trevor-Robinson all you have to run now is rails standard:fix to get the linter to pass :)

Trevor-Robinson commented 1 month ago

Should be all good now.

kasugaijin commented 1 month ago

@Trevor-Robinson Just a heads up - I missed it in the review, but there were some issues with the naming you used on migrations. First, I am not sure why you changed the class names of the two existing migrations from PageText to CustomPage. You should never change an existing migration (the only exception might be if it's the latest one). But this can cause a lot of unintended consequences. You are better of making changes through new migrations.

Also, the naming of the latest migration was wrong - i.e. the filename should always match the class name so Rails knows where to find the class when you call it. See below.

These are not major issues and easy to fix, but I wanted to provide feedback. Also, I am not sure how the schema got updated with the new table name despite these migration errors...the migrations should have failed. So, did you update the file manually by chance? That's also a red flag if you had to do that.

image