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

Messaging #132

Merged
merged 31 commits into from Dec 14, 2021
Merged

Messaging #132

merged 31 commits into from Dec 14, 2021

Conversation

joemasilotti
Copy link
Owner

@joemasilotti joemasilotti commented Dec 3, 2021

This PR addresses #120, #121, and #122. I tried to split this into 3 PRs, but I ended up rewriting so much of #130 that I don't think it is worth keeping them separate anymore.

All of the refactors from #130 have been pulled in, so check out that PR for details on the pundit changes.

I ended up simplifying the routes a ton to get the base functionality working. Instead of trying to nest conversations inside of developers, the route is more Rails-y: /conversations/:id. This helps clean up the controller logic a ton.

On top of that, to ensure only a Business can initiate a conversation, ColdMessagesController was added. This includes the logic for kicking off that first message and redirecting to an existing conversation if one exists.

Room for improvement / future PRs

  • The Message form at the bottom of the conversation page could be Turbo-ified
  • The new Business page is very barebones, and could use another design pass
    • There's probably new fields a Business wants to add, too

Pull request checklist

  • Your code contains tests relevant for code you modified
  • All new and existing tests are passing
  • You have linted the project with bundle exec standardrb --fix
  • You have linted the project with bundle exec erblint --lint-all --autocorrect

Closes #120, #121, and #122.

@joemasilotti joemasilotti changed the base branch from main to message-a-developer December 3, 2021 05:30
@joemasilotti joemasilotti changed the base branch from message-a-developer to main December 8, 2021 00:14
@joemasilotti joemasilotti mentioned this pull request Dec 8, 2021
17 tasks
@joemasilotti joemasilotti changed the title Message a business Messaging Dec 8, 2021
@joemasilotti joemasilotti marked this pull request as ready for review December 8, 2021 03:50
@joemasilotti
Copy link
Owner Author

CC @jacobdaddario and @Tonksthebear because you both expressed interest in these features. Would love to hear your thoughts or quick PR review before I merge.

@jacobdaddario
Copy link
Contributor

Hey! Sorry I've been occupied with some other stuff recently. I'll take a look this evening!

@Tonksthebear
Copy link
Contributor

Hey, I have a lot on my plate this week. I might be able to hop on and take a look, but I just wanted to acknowledge your message and give you a heads up to not wait on me. I will definitely take a look if I get a chance though!

@jacobdaddario
Copy link
Contributor

Hey, sorry for not getting to this yesterday. Life has still been kicking my butt. I'll attempt to get to it tonight.

@jacobdaddario
Copy link
Contributor

Looks good to me! I think there's things I would have approached a different way, but the logic you laid out all seems to make sense.

@joemasilotti joemasilotti merged commit e1903c4 into main Dec 14, 2021
5 checks passed
@joemasilotti joemasilotti deleted the message-a-business branch December 14, 2021 22:18
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.

A business can message a developer
3 participants