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 the "internal" flag to NotesApi.createIssueNote() #1029

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

hannibal218bc
Copy link
Contributor

  • add a new createIssueNote() method allowing to pass the internal flag.
  • add the internal flag to the Note model

Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add the internal flag to the example file:
https://github.com/gitlab4j/gitlab4j-api/blob/main/src/test/resources/org/gitlab4j/api/note.json
--> this should be as close as possible to a real object returned by GitLab.

This way this test make sure our model matches with the JSON example:

@Test
public void testNote() throws Exception {
Note note = unmarshalResource(Note.class, "note.json");
assertTrue(compareJson(note, "note.json"));
}

... and your addition is covered checked by a test.

src/main/java/org/gitlab4j/api/NotesApi.java Outdated Show resolved Hide resolved
src/main/java/org/gitlab4j/api/NotesApi.java Outdated Show resolved Hide resolved
@jmini
Copy link
Collaborator

jmini commented Sep 8, 2023

@hannibal218bc thank you for this PR. Small changes are requested but the change looks almost good to me.

@hannibal218bc
Copy link
Contributor Author

@hannibal218bc thank you for this PR. Small changes are requested but the change looks almost good to me.

@jmini thank you for the feedback. I'll submit the changes as soon I as time permits.

@hannibal218bc hannibal218bc force-pushed the heek-feature-notesapi-internal branch from 65da884 to 4b40848 Compare September 22, 2023 20:21
@hannibal218bc hannibal218bc requested a review from jmini September 22, 2023 20:23
@hannibal218bc hannibal218bc force-pushed the heek-feature-notesapi-internal branch from 4b40848 to 3faec00 Compare September 22, 2023 20:26
@hannibal218bc
Copy link
Contributor Author

@jmini thank you for the review, I've updated the MR with the requested changes.

Please also add the internal flag to the example file: https://github.com/gitlab4j/gitlab4j-api/blob/main/src/test/resources/org/gitlab4j/api/note.json --> this should be as close as possible to a real object returned by GitLab.

I cross-checked the file against a file retrieved from a 16.4 enterprise instance, and there are a few more differences - added attachments, confidential, but author.created is missing. And a new field type that isn't even documented at Gitlab...

I guess the JSON shall only contain properties that are actually supported by Gitlab4J?

@jmini
Copy link
Collaborator

jmini commented Sep 25, 2023

I guess the JSON shall only contain properties that are actually supported by Gitlab4J?

Sadly this is the long work of keeping our model aligned with the JSON returned by GitLab. Step by step we are getting closer.

I agree that the field you mention are out of scope for this pull request.

And a new field type that isn't even documented at Gitlab

This is crazy. They have also issue keeping up-to-date their doc with their code.

Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. I will do some manual testing before merging.

jmini added a commit to jmini/gitlab-experiments that referenced this pull request Sep 26, 2023
@jmini jmini merged commit fac578c into gitlab4j:main Sep 26, 2023
@hannibal218bc
Copy link
Contributor Author

It looks good to me. I will do some manual testing before merging.

Thank you @jmini !

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