Closed jdmchugh111 closed 2 months ago
1) This is a GIANT PR. Typically I'd prefer to see each action (index, show, create, update, destroy) as its own PR. It's really hard to review big PRs well. Smaller ones set us up for better success.
2) I'm curious why you chose to put everything in a TagsController
instead of a LinkTagsController
for the CRUD functionality of adding tags to links?
3) Nice work.
On 1. - I definitely agree that it's very big for one PR. I just wanted to set up the FE team to be able to integrate the backend tagging endpoints asap. Would it have made more sense to separate endpoints into their own PRs and then open multiple PRs all at once?
On 2. - I was mostly going for ease of use / developer-friendly so I thought the api/v1/tags made most sense for the base path of the url, and Rails routes this to the TagsController by default. I can go in and set a custom controller so that they are handled in a LinkTagsController instead. I can also change the path for the endpoints if you think the current one doesn't feel conventional or should be something more like api/v1/link_tags instead
I'll wait to merge this until your questions are answered James but I think this looks really good overall. everything made sense to me, I'm in a good place to get started on the next endpoints.
Noah if you’re good with this go ahead and merge.
James- yeah ideally each endpoint would’ve been its own PR but it’s not worth the effort to do that this time around. Just something to keep in mind for future PRs. As for the other question (#2) that’s a design decision thing. It’s slightly unrestful but you’re allowed to make choices that break convention. I just need you to be able to talk about why. You have a reason for why you did it that way. No need to change.
Description of Changes
Migrations for Tags table and LinkTags joins table. All Tags seeded in seeds.rb. All 4 tagging endpoints functional and fully tested
What are the relevant tickets (if any)
closes #20 closes #14 closes #18 closes #16 closes #23 closes #17
Testing
Unit testing for models. Integration testing for requests - all tests passing and coverage is 100%
Checklist