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

Add pull request reviews, pull request review comments and pull request review comments (yes, that's right) #1859

Open
wants to merge 49 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@BenJam
Copy link
Contributor

commented May 28, 2019

As above. WIP

BenJam added some commits May 28, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

uses #1141 in fixtures to test (so check out that thread for expected behaviour). Does not support commit-level comments.

@andrew
Copy link
Member

left a comment

Here's a review comment for you!

@andrew
Copy link
Member

left a comment

And a review (With a comment)

@BenJam BenJam marked this pull request as ready for review May 28, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

hmmmm, this is currently storing a lot of 'no description given' comments where there are body-less reviews/comments

@BenJam BenJam changed the title Add pull request reviews and pull request review comments Add pull request reviews, pull request review comments and pull request review comments (yes, that's right) May 28, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

test currently seems to be skipping update_comments in self.sync. Any pointers appreciated if anyone gets a look see.

BenJam added some commits May 29, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

back at this: looks like this isn't calling download comments because it's not getting a gh client.

BenJam added some commits Jun 3, 2019

fix review test ordering
add include comments stub
@andrew

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Comment syncing is still optional and enabled with INCLUDE_COMMENTS=1 ENV var, so it'll need to be turned on in the tests that should sync comments, at least test/models/subject_test.rb and test/controllers/hooks_controller_test.rb, with the following code:

setup do
  stub_include_comments
end
Show resolved Hide resolved test/models/subject_test.rb Outdated

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019 — with Octobox

ta!

BenJam added some commits Jun 4, 2019

Add subject 58 comments fixture
stub comments request
stub include comments env var
remove methods stubs

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019 — with Octobox

it's all one comment stream atm.

@andrew

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@BenJam perhaps we can add store the state field if it's present (which it only will be for reviews) and then add the relevant icon to the header of the comment if it's present.

BenJam added some commits Jun 7, 2019

Add review state to comments regarding a review where there's a state…
… other than 'COMMENTED'. Note: we replace those with the corresponding comment(s)
Show resolved Hide resolved app/models/subject.rb Outdated

BenJam added some commits Jun 10, 2019

use map rather than .each to iterate over reviews
move stubs to stub helper
[todo] tidy stub request urls for regex's
rubocop of shame
stub all commit status requests

@andrew andrew self-requested a review Jun 11, 2019

@andrew

andrew approved these changes Jun 17, 2019

@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@chrisarcand I can't see anything that's going to screw up your PR significantly by merging this. I'd quite like to get it out and move everyone onto comment-threads asap.

@chrisarcand

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

For sure, go for it, looks great!

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019 — with Octobox

Cool, just going to address a little bug in which reviews aren't stored with the correct datetime

BenJam added some commits Jun 18, 2019

fix typoe for approved reviews
ensure that created dates are stored for reviewed (send at submitted at)
@BenJam

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Such meta:

Screenshot 2019-06-18 at 13 51 12

BenJam added some commits Jun 18, 2019

Update comments count with total number of comments (inc. review comm…
…ents)

signify review comments and reviews in expand button

@octobox octobox deleted a comment from bertie Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.