add Sprint column for issue. Fix get group member function to return all members. #119

zenixls2 · 2018-03-12T08:59:20Z

Change api use of Issue.Get from
https://developer.atlassian.com/cloud/jira/platform/rest/#api-api-2-issue-issueIdOrKey-get
to
https://docs.atlassian.com/jira-software/REST/7.3.1/#agile/1.0/issue-getIssue
in order to export Sprint column
Fix related test cases
Export IsLast and NextPage for cases that members' length is more than one page.
Call Group.Get to iterate through all members instead of returning only the first page
(we have no way to exam if there's other pages of members from this api design, so return all by default)

zenixls2 · 2018-03-12T09:16:59Z

It seems accessing https://issues.apache.org/jira/rest/agile/1.0/issue/MESOS-3325 needs to be authenticated...

rbriski · 2018-03-12T15:41:49Z

This change will affect everyone else using the library. You're changing the version of the API that we use for pulling issues. It also uses the AuthenticationService to determine which version of the API to use. That seems fragile. If you're going to change the endpoint version, should we version the entire service as well?

rbriski · 2018-03-12T15:51:52Z

I looked at this again and you're switching to the agile endpoint. This means everyone will have to switch to the agile endpoint. I don't think we should force everyone to a non-standard endpoint just to include agile feature like boards and sprints. Please move the agile endpoint into a separate entity instead of taking over the Issue entity.

Also, I like the idea of pagination support but for everything, not just a one-off for groups. Github handles pagination nicely (https://github.com/google/go-github/blob/2f5b584d4694ff036b98283940be4d23d113e2d4/github/github.go#L352). This doesn't translate exactly to how JIRA handles it but I think it's a good start for a nicer solution that covers all cases.

zenixls2 · 2018-03-13T01:49:42Z

@rbriski Do you mean like the implementation for Sprint and Board APIs? I'm not sure what entity name it should be if I create a separate service for agile endpoints. Also, I'm not sure if creating a new service would be a right way for implementing the agile endpoints. BTW, is agile endpoint not stable or not ready for production use, as you have mentioned that it is non-standard?

And yes, I agree that pagination support is quite important for many cases in go-jira. However, only getting the first page of something doesn't make sense for any use case.

For the request for pagination, actually sb. has already reported in issues (just listed here for reference):
#55
need some plans for backward compatibility, since this modification will sure influence almost all users.
(Or just create a new version, like go-jira 2.0, to support new features)
If we really want to implement pagination in this pull request, please give me any advice on the backward compatibility part. The returned type for many APIs might have to be changed.

rbriski · 2018-03-13T05:06:50Z

Adding Sprint to the IssueFields struct should be fine. If the field is not there, it should just omit it since you have the omitempty flag set.

By "non-standard" I just mean that it may not be available to everyone. Is the agile endpoint available for projects that aren't agile? What about service desk projects? It seems like a lot of unknowns to just be dropping a new endpoint in.

You could do something like GetWithAgile and replicate the Get method but with the agile endpoint. Are there additional agile endpoints? Should we consider letting the base endpoint be settable? I'm not exactly sure but I'd like to consider the larger picture rather than fixing a single use case.

Regarding pagination, yes, it should be there but I don't think this is the best way. I haven't looked closely enough to figure out a backward compatible way. I'll have more time this weekend.

If you figure out a good way to add the agile "GET", just strip out the pagination stuff for now and I can merge this. Even if you do have a good idea of pagination, it would be best to have it in a separate PR.

rbriski · 2018-03-13T05:11:16Z

Looking at this more, the Agile add-on encompasses a lot more calls:
https://docs.atlassian.com/jira-software/REST/7.3.1/

You could add Sprint to the struct but I think I'd create an entirely new service just to handle agile stuff.

zenixls2 · 2018-03-13T10:02:11Z

The modification I applied:

split out agile endpoint with another API
create another function for group pagination only. Left original function for backward compatibility.
add test for the group pagination
Only one thing needs to be confirmed: groupMembersResult is a private struct, so it won't be shown in godoc. New GetNextPage function won't be shown either. Maybe we should make it public?

rbriski

I've been thinking about this a lot for the past few days and I have some ideas that I've laid out below. Tell me if you want me to elaborate more. I know it's been a little scattered but I think I've landed on the best solution right now.

rbriski · 2018-03-15T18:20:39Z

group.go

+// User of this resource is required to have sysadmin or admin permissions.
+//
+// JIRA API docs: https://docs.atlassian.com/jira/REST/server/#api/2/group-getUsersFromGroup
+func (s *GroupService) GetByPage(name string, startAt int, maxResults int, includeInactiveUsers bool) (*groupMembersResult, *Response, error) {


Can you rename this to GetWithOptions and have the method signature match this:
https://github.com/andygrunwald/go-jira/blob/master/issue.go#L783

That's a good example of how pagination is set up in this library. I'm not sure why it wasn't included in the GroupService but ... here we are. If there's a major version change in the future, I would expect options to be included in the Get function.

Sure, this make sense. Would better to wrap these in options.

rbriski · 2018-03-15T18:23:23Z

issue.go

+//
+// JIRA API docs: https://docs.atlassian.com/jira-software/REST/7.3.1/#agile/1.0/issue-getIssue
+// TODO: create agile service for holding all agile apis' implementation
+func (s *IssueService) GetWithAgile(issueID string, options *GetQueryOptions) (*Issue, *Response, error) {


Can you put this in the SprintService? I'd like to keep all of the agile-specific stuff together. I think naming it GetIssue will make sense in that service. You can also get rid of the if/else stuff for the endpoint because they'll know exactly what they're calling if they use SprintService

actually board also uses agile api. are we going to merge board to somewhere else?
Moreover, the agile api doesn't actually only add sprint information. It also comes out with the epic infor. I think it might not be proper to put in SprintService.

rbriski · 2018-03-15T18:27:23Z

group.go

@@ -18,6 +18,30 @@ type groupMembersResult struct {
 	MaxResults int           `json:"maxResults"`
 	Total      int           `json:"total"`
 	Members    []GroupMember `json:"values"`
+	IsLast     bool          `json:"isLast"`


Please remove these. They're not needed in this results struct.

rbriski · 2018-03-15T18:29:35Z

group.go

+	Client     *Client       `json:"-"`
+}
+
+func (g *groupMembersResult) GetNextPage() (*groupMembersResult, *Response, error) {


Can you remove this function? I don't think we need to copy the GetByPage function. The code to set up the next page can be in the calling code's loop like this:
https://github.com/google/go-github#pagination

rbriski

The pagination stuff looks good. Just a request to use GetWithOptions since GetByOption doesn't make sense.

Regarding the SprintService, I agree that it's not ideal, but it's better than mixing the agile stuff in with the main API. There aren't any other functions named "Agile" and SprintService.GetIssue keeps the agile stuff together. I know there's a BoardService but that seems more for kanban (maybe?). JIRA certainly makes it confusing.

rbriski · 2018-03-16T13:45:35Z

group.go

+// User of this resource is required to have sysadmin or admin permissions.
+//
+// JIRA API docs: https://docs.atlassian.com/jira/REST/server/#api/2/group-getUsersFromGroup
+func (s *GroupService) GetByOption(name string, options *GroupSearchOptions) ([]GroupMember, *Response, error) {


GetWithOptions

rbriski · 2018-03-27T20:23:30Z

@zenixls2 are you still working on this?

zenixls2 · 2018-03-27T23:21:41Z

sorry, i was quite busy during these days, and maybe until next week. I'll fix this when i have time.

…Options

zenixls2 · 2018-04-03T05:33:48Z

hi, I've changed the code according to your comments

zenixls2 added 8 commits November 21, 2017 20:14

use agile api for getting issue

ce5e0c2

try fix group error

8621b5f

revert

9862cae

get all users at once

4651c95

Merge branch 'master' of https://github.com/andygrunwald/go-jira

6b38102

fix test, merge from upstream.

6e4048b

remove debug code

b5ebe58

remove debug code

de750e2

zenixls2 added 2 commits March 12, 2018 18:24

if not authenticated, fallback to use api/2 for issue get

98ec0fc

since test is never authenticated, fallback to api/2 endpoint

958c218

split out agile endpoint for issue get, add api for pagination support

1d38c6c

rbriski suggested changes Mar 15, 2018

View reviewed changes

use pagination from design

04cbb2f

rbriski suggested changes Mar 16, 2018

View reviewed changes

zenixls2 added 2 commits April 3, 2018 11:08

Merge branch 'master' of https://github.com/andygrunwald/go-jira

8a9b83c

move agile issue api to sprint, change name of GetByOption to GetWith…

18de7ff

…Options

rbriski approved these changes Apr 3, 2018

View reviewed changes

rbriski merged commit 7f227a2 into andygrunwald:master Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Sprint column for issue. Fix get group member function to return all members. #119

add Sprint column for issue. Fix get group member function to return all members. #119

zenixls2 commented Mar 12, 2018

zenixls2 commented Mar 12, 2018

rbriski commented Mar 12, 2018

rbriski commented Mar 12, 2018

zenixls2 commented Mar 13, 2018

rbriski commented Mar 13, 2018

rbriski commented Mar 13, 2018

zenixls2 commented Mar 13, 2018

rbriski left a comment •

edited

Loading

rbriski Mar 15, 2018

zenixls2 Mar 16, 2018

rbriski Mar 15, 2018

zenixls2 Mar 16, 2018

rbriski Mar 15, 2018

rbriski Mar 15, 2018

rbriski left a comment

rbriski Mar 16, 2018

rbriski commented Mar 27, 2018

zenixls2 commented Mar 27, 2018

zenixls2 commented Apr 3, 2018

add Sprint column for issue. Fix get group member function to return all members. #119

add Sprint column for issue. Fix get group member function to return all members. #119

Conversation

zenixls2 commented Mar 12, 2018

zenixls2 commented Mar 12, 2018

rbriski commented Mar 12, 2018

rbriski commented Mar 12, 2018

zenixls2 commented Mar 13, 2018

rbriski commented Mar 13, 2018

rbriski commented Mar 13, 2018

zenixls2 commented Mar 13, 2018

rbriski left a comment • edited Loading

Choose a reason for hiding this comment

rbriski Mar 15, 2018

Choose a reason for hiding this comment

zenixls2 Mar 16, 2018

Choose a reason for hiding this comment

rbriski Mar 15, 2018

Choose a reason for hiding this comment

zenixls2 Mar 16, 2018

Choose a reason for hiding this comment

rbriski Mar 15, 2018

Choose a reason for hiding this comment

rbriski Mar 15, 2018

Choose a reason for hiding this comment

rbriski left a comment

Choose a reason for hiding this comment

rbriski Mar 16, 2018

Choose a reason for hiding this comment

rbriski commented Mar 27, 2018

zenixls2 commented Mar 27, 2018

zenixls2 commented Apr 3, 2018

rbriski left a comment •

edited

Loading