added fields in GHPullRequestReviewComment which were missing

[kisaga]

Description

added new fields in GHPullRequestReviewComment that were missing. Assuming that the changes are minor the mocked data snapshot has not been updated.

The response and its JSON Schema can be found here fixes #1463

Before submitting a PR:

• [x] Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
• [x] Add JavaDocs and other comments as appropriate. Consider including links in comments to relevant documentation on https://docs.github.com/en/rest .
• [x] Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
• [x] Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
• [x] Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

• [x] Fill in the "Description" above with clear summary of the changes. This includes:
• [x] If this PR fixes one or more issues, include "Fixes #" lines for each issue.
• [x] Provide links to relevant documentation on https://docs.github.com/en/rest where possible.
• [x] All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
• [x] Enable "Allow edits from maintainers".

[gsmet]

Assuming that the changes are minor the mocked data snapshot has not been updated.

I think this is gonna be a problem. We need to be able to regenerate the mocked data and still have the tests passing. You should probably create a test project in the test org, and add a review there that allows to test this thing.

[gsmet]

Assuming that the changes are minor the mocked data snapshot has not been updated.

I think this is gonna be a problem. We need to be able to regenerate the mocked data and still have the tests passing. You should probably create a test project in the test org, and add a review there that allows to test this thing.

[kisaga]

Cool! I'll check how to create a project in the test org and how to update the mock data.

[kisaga]

Hi @gsmet I've run the tests against the https://github.com/hub4j-test-org/github-api twice:

• https://github.com/hub4j-test-org/github-api/pull/451
• https://github.com/hub4j-test-org/github-api/pull/452

And I have some questions:

1. In the Contributing guide it is mentioned Please request access to this org to record your tests before submitting a PR.. From whom should I request access ? The two tests I've run are failing at the validation of AuthorAssociation field because mine is NONE but the expected is MEMBER.
2. Is it a problem if I continue using this project to modify the setup phase of the test I've modified ? (I need to add some reactions to get the respective field in the response)
3. For the final snapshot that will be committed what should be the target repository for the responses ?

Waiting for your feedback :slightly_smiling_face:

[kisaga]

Hi @gsmet, I managed to take a snapshot after modifying the test to add some reactions, fetch again the comment and check the reactions. I've also modified some values that I had hardcoded in the mock responses in my initial approach. The snapshot is from this PR and I have modified manually the author_association from NONE to MEMBER. I don't know if the snapshot is fine or if it should be taken as a MEMBER, but even if this the case, if I become member of the test org I can take a new snapshot and commit the changes.

Let me know if it is fine or if I should take the snapshot as a MEMBER (if yes, how can I join the test org).

[bitwiseman]

@kisaga I've invited you to the test org. You should be able to take Snapshots as a member now.

[codecov[bot]]

Codecov Report

Base: 79.56% // Head: 79.63% // Increases project coverage by +0.07% :tada:

Coverage data is based on head (83324ca) compared to base (24832b1). Patch coverage: 92.30% of modified lines in pull request are covered.

:exclamation: Current head 83324ca differs from pull request most recent head 248e4f2. Consider uploading reports for the commit 248e4f2 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1469      +/-   ##
============================================
+ Coverage     79.56%   79.63%   +0.07%     
- Complexity     2166     2187      +21     
============================================
  Files           207      208       +1     
  Lines          6616     6655      +39     
  Branches        364      364              
============================================
+ Hits           5264     5300      +36     
- Misses         1140     1141       +1     
- Partials        212      214       +2     

Impacted Files	Coverage Δ
...org/kohsuke/github/GHPullRequestReviewComment.java	88.00% <84.21%> (-1.29%)	:arrow_down:
...ke/github/GHPullRequestReviewCommentReactions.java	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[bitwiseman]

💢 So, the reactions field shows up in the "response schema" on that section but not in the "example response". :(

That page gives me no indication of the right path forward. :( Not your fault. I'm concerned because the "Reaction API" paged don't talk about this field. Is it because it is new and they haven't documented it, or because it is old and will eventually be replaced? I do not know.

https://docs.github.com/en/rest/pulls/comments#get-a-review-comment-for-a-pull-request

...
    "reactions": {
      "title": "Reaction Rollup",
      "type": "object",
      "properties": {
        "url": {
          "type": "string",
          "format": "uri"
        },
        "total_count": {
          "type": "integer"
        },
        "+1": {
          "type": "integer"
        },
        "-1": {
          "type": "integer"
        },
        "laugh": {
          "type": "integer"
        },
        "confused": {
          "type": "integer"
        },
        "heart": {
          "type": "integer"
        },
        "hooray": {
          "type": "integer"
        },
        "eyes": {
          "type": "integer"
        },
        "rocket": {
          "type": "integer"
        }
      },
      "required": [
        "url",
        "total_count",
        "+1",
        "-1",
        "laugh",
        "confused",
        "heart",
        "hooray",
        "eyes",
        "rocket"
      ]
    },
...

[kisaga]

HI @bitwiseman I've asked a question here. We can wait for an answer and then make our decision for this field. If we need the rest of the changes we can:

• either include the reactions field and remove it if the answer will be that it'll removed at some point. In this case if it will be deprecated it will have a null value
• or exclude it and add it in case the answer is that it will not be removed

[bitwiseman]

Please exclude it until we have an answer. It might make sense to start a new PR for the other changes, since they shouldn't even require new test data, just some additional asserts.

[bitwiseman]

@kisaga Please make the change requested above so we can merge the rest of this PR.