Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Base Ability to Have Custom Fields #153

Closed
wants to merge 11 commits into from
Closed

Base Ability to Have Custom Fields #153

wants to merge 11 commits into from

Conversation

tthornton3-chwy
Copy link
Contributor

@tthornton3-chwy tthornton3-chwy commented Jun 28, 2023 •
edited
Loading

It puts the additionalFields as an option, to return from Jira more fields than the default. This includes the proper tests necessary to highlight functionality.

@tomasbjerre
Copy link
Owner

I just spent a little time reviewing this. My general comment here is that it looks complicated and not something I want to maintain. Actually I have been considering dropping these integrations as I feel they are not properly tested and adds complexity to the library.

Perhaps this is one of those cases where you are better off publishing your own fork.

@tthornton3-chwy
Copy link
Contributor Author

tthornton3-chwy commented Jun 28, 2023 •
edited
Loading

Dear @tomasbjerre, thank you so much for your quick review-- having a helpful library used by so many still super actively maintained/looked by the original dev seems super rare.

And your feedback on it being too complicated is completely fair and reasonable. If you can call out some places where things go off the rails here, I would love to come back to this and simplify it / break it down into simpler methods / add more comments, etc :) I know for example I use Streams at one point, that could've easily of been a for loop instead, so if that's what you are referring too i'm happy to do that instead (i'm just a Stream-fan).

To your comment about dropping the Jira integration, you're totally within your right to do so. I can say that a lot of people that i've seen online LOVE this aspect of the library, especially when this is used with the jenkins plugin. It's really nice!!! Instead, could we together highlight where you feel like the testing is lacking, and I could come in and write some more unit tests for it, so we're both feeling comfortable? I could also just jump in and take this one, and start throwing more in if we'd like.

And lastly, I could definitely make my own fork of this, my hope is we can hammer this sucker out and get it to a good place, any comments/concerns with bringing it in here, but if this is just not something you're interested in I'll take that route then :)

tomasbjerre
tomasbjerre reviewed Jun 30, 2023
View reviewed changes
src/main/java/se/bjurr/gitchangelog/internal/integrations/jira/JiraClient.java Outdated
@@ -25,30 +52,106 @@ public String getApi() {
return this.api;
}

protected String getIssuePath() {
return this.hasIssueFieldFilters() ? SEARCH_API : ISSUE_API;
Copy link
Owner

Choose a reason for hiding this comment

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

Can you not always use search API? When can you use it? When can you not use it?

Copy link
Contributor Author

@tthornton3-chwy tthornton3-chwy Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you would use search vs issue for when you are wanting an issue returned to potentially be skipped. I tested this w/ Postman by adding essentially an assignee = 'me@email.com' and seeing the ticket either showing up or not.

tomasbjerre
tomasbjerre reviewed Jun 30, 2023
View reviewed changes
src/main/java/se/bjurr/gitchangelog/api/GitChangelogApi.java
@@ -513,6 +514,33 @@ public GitChangelogApi withRedmineUsername(final String string) {
return this;
}

/**
Copy link
Owner

Choose a reason for hiding this comment

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

I still don't understand this feature. Are you looking for issues that are not mentioned in the commit message?

When would I want to use this method and what would be the result?

Perhaps these are 2 methods are 2 completely separate features? In that case it might be easier to understand the PR if it is split into 2.

Copy link
Contributor Author

@tthornton3-chwy tthornton3-chwy Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Great question haha this feature is also not one that I ever need for my team's workflow, but according to the original author of #97:

This feature is needed, if you develop for e.g. multiple clients in one branch and in one Jira project, but each client should only see the commits that belong to him. So you can define a custom field and integrate this into the plugin configuration and filter out all other tickets that are of no interest.

That made kinda sense to me, maybe? I believe you would use search vs issue for when you are wanting an issue returned to potentially be skipped. I tested this w/ Postman by adding essentially an assignee = 'me@email.com' and seeing the ticket either showing up or not.

I am with you that this can be separated into two different PRs, essentially a 'fixed' #97 and then the "additional fields" work I added on top. Since my stuff is what's important to me, this PR will be rebased to only include the additional fields.

@tthornton3-chwy tthornton3-chwy changed the title #97 + Base Ability to Have Custom Fields Base Ability to Have Custom Fields Jun 30, 2023
tomasbjerre
tomasbjerre reviewed Jul 1, 2023
View reviewed changes
src/main/java/se/bjurr/gitchangelog/internal/integrations/jira/JiraClient.java
protected String getIssuePath() {
return ISSUE_API;
}

protected String getEndpoint(final String issue) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this method should me unit-tested.

tomasbjerre
tomasbjerre reviewed Jul 1, 2023
View reviewed changes
src/main/java/se/bjurr/gitchangelog/internal/integrations/jira/JiraClient.java
}

private String getFieldPrefix() {
return ISSUE_API_FIELD_PREFIX;
}

protected JiraIssue toJiraIssue(final String issue, final String json) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this method should me unit-tested.

@tomasbjerre
Copy link
Owner

I think it looks good now, would be nice with some unit tests on those 2 methods I commented on.

tomasbjerre pushed a commit that referenced this pull request Jul 2, 2023
@tthornton3-chwy @tomasbjerre
feat: enable additional fields from jira integration (refs #153)
705da81
@tomasbjerre
Copy link
Owner

I squashed your commits and merged manually.

Released in 1.170.0: https://github.com/tomasbjerre/git-changelog-lib/blob/master/CHANGELOG.md#11700-2023-07-02

Thanks!

@tomasbjerre tomasbjerre closed this Jul 2, 2023
@tthornton3-chwy
Copy link
Contributor Author

Thank you for reviewing and merging this in :) I super appreciate seeing it in-- I can work on those unit tests during the follow-up PR that adds the functionality of #97 in on top of this!
I got the bump + flags over in tomasbjerre/git-changelog-command-line#13 then, for this, whenever we got time.

@tomasbjerre
Copy link
Owner

Thank you for reviewing and merging this in :) I super appreciate seeing it in-- I can work on those unit tests during the follow-up PR that adds the functionality of #97 in on top of this! I got the bump + flags over in tomasbjerre/git-changelog-command-line#13 then, for this, whenever we got time.

I added the unit tests. So dont worry about that.

Im still sceptical about the other feature in #97 if only very few users need it I dont think it is worth the added complexity. Making the code harder to maintain...

@tthornton3-chwy
Copy link
Contributor Author

I think I agree with you-- this PR adds a feature I feel a bunch of people will probably use, but I couldn't find many uses for the latter/#97. I feel a bit bad for the OP, but if it's a feature anyone else recommends I can submit the PR I got stashed. Thanks again!

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@tomasbjerre tomasbjerre tomasbjerre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tthornton3-chwy @tomasbjerre