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

Post comments directly to a subject from Octobox #1523

Open
wants to merge 88 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@BenJam
Copy link
Contributor

BenJam commented Jan 24, 2019

Adds a form to the bottom of the thread view to post a comment on that subject.

  • adds form, controller, model methods et al to accept, process and render the comment
  • adds comment worker, working a new comment queue (could add this to current user queue)
  • escapes shortcuts if we're focussed on the comment box

I am in two minds as to whether to bother adding a JS action on this as turbolinks is doing a good job of re-rendering. But will happily add if needed.

Future extensions of this might add previews for markdown, file uploading etc. this is just a starter to build upon.

Will need some work to #1484 to ensure that comments aren't re-rendered when receiving the hook back from GH which I'll add there this avo.

BenJam added some commits Jan 9, 2019

* add button styles again
* link button on thread to original issue
+ make avatars smaller
* add timeline thread
* add issue number to threads
* add comment count to threads
speed up bootsrtrap collapse transitions
remove extra border on collapsed cards
save comment to db
post comment to gh
sync the comment in the db
Show resolved Hide resolved app/models/repository.rb Outdated
Show resolved Hide resolved app/models/repository.rb Outdated

BenJam and others added some commits Feb 8, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

BenJam commented Feb 13, 2019

picking up the post-token stuff later today after a couple of days working on boardgames 👍

@BenJam

This comment has been minimized.

Copy link
Contributor Author

BenJam commented Feb 13, 2019

wondering why the heck I have a test failing on a dependency bump merge...

@andrew

This comment has been minimized.

Copy link
Member

andrew commented on 0e0eefd Feb 13, 2019

Would be good to have a test for this

This comment has been minimized.

Copy link
Member

andrew replied Feb 15, 2019

It might be easier to test if you extra the logic that picks the client into a separate method, perhaps on User that looks something like:

def comment_client(comment)
  if user.app_token.present? && comment.subject.repository.commentable?
    user.app_installation_client
  else
    github_client
  end
end

Then you can write a couple tests like:

"for user with an app_token, commenting on a commentable repo uses app_installation_client"
"for user without an app_token, commenting on a commentable repo uses github_client"
"for user with an app_token, commenting on a non-commentable repo uses github_client"

Then in each test, you can assert the return value of user.comment_client(comment) to be either user.github_client or user.app_installation_client

@BenJam

This comment has been minimized.

Copy link
Contributor Author

BenJam commented Feb 15, 2019

wondering why the heck I have a test failing on a dependency bump merge...

Looks like this erroring out on a reset back to b4e9d57

BenJam added some commits Feb 15, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

BenJam commented Feb 15, 2019

Would be good to have a test for this

not sure how to test this. capturing the post from subject.comment_on_github and checking against the users key would be my go to. But I'm not sure how to do that.

BenJam added some commits Feb 18, 2019

@BenJam
Copy link
Contributor Author

BenJam left a comment

I think these are all sorted now.

Show resolved Hide resolved app/models/user.rb Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment