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

Discussions CRUD + Listing #22

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

practicingruby
Copy link
Contributor

Hi Jordan, we aren't ready to merge upstream yet, but I wanted to do an interim review request just so that we can learn a few things and roll in some of your suggestions before we do merge.

Jia and I have worked on making it so discussions can be associated with an author. To do this, we ended up having to do a bit of gravatar integration and refactoring of PersonDecorator, but we made those changes on master because they seemed to be generally useful.

After making those changes, we updated the code on our feature branch to implement authorship and gravatars, as shown by the screenshot below.

I left a few questions for you on one of our commits, but would be happy to hear any other feedback you have as well.

NOTE: In order to run this code it is necessary to update any discussions from our original seed data using something like:

Discussion.all.each { |a| a.update_attribute(:author, "sandal") }

We plan to add very simple discussion CRUD before merging, and delete the db/seeds.rb file. But I figured I'd let you know about this gotcha in the interim.

@practicingruby
Copy link
Contributor Author

@jordanbyron: We are spiking on discussions CRUD right now because we want to get a feel for how that will affect the main listing page. Please don't bother reviewing until we clean things up a bit. Or if you want to give us some feedback, focus on the first commits related to integrating gravatar. Answering my questions on those will help me sort of whether we've been doing things right in our more recent commits or not.

@jordanbyron
Copy link
Contributor

@sandal no problem. Let me know when this is ready for me to check out 👀

@practicingruby
Copy link
Contributor Author

Hi Jordan,

Now that we've reached the end of the course, its time for us to wrap up this pull request, I suppose. What we have here is a good proof of concept (and likely a usable implementation) of the discussion listing page, and some VERY rough CRUD code for the creation and editing of new discussions. There are a number of limitations to this code to be aware of:

  • Currently we use an HTTP DELETE to archive posts. We have UI elements to close (archive) a discussion, but none to open them. There is no UI for deletion of discussions, because I haven't decided how that should work.
  • Currently there is no access control, all discussions can be created and edited by everyone
  • We did not make any progress at all on comments
  • We enabled HTML in discussion bodies as a proof of concept, but really we'll want to do Markdown instead here.
  • We have gravatars for people in discussions, but they do not currently link to anything (this depends on Chris + Vinicius's work)
  • The DiscussionListDecorator can probably be refactored quite a bit
  • There are no tests!

It's up to you how you want to go forward with this. I think it definitely would be possible to gradually refactor this code into what we will end up using for real, but it's not close to being in a usable state yet. In any case, additional testing would be essential as we move forward. Right now most of the interesting logic is in the decorators, but I haven't sorted out the best way to test them, especially considering that the structure might change quite a bit.

My rough idea for what might be the right approach is to get some feedback from you on this code, merge it quickly to prevent bitrot, and then make aggressive cuts to it as needed when we actually start working on this feature for real. During that time, we'd shore up the testing and make some decisions about the things that have been left undone so far. We could probably work on this together over the next couple weeks, if that sounds good to you.

Let us know what you think, and please give some feedback on what we built when you get a chance!

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.

2 participants