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

Discussion: Marking existing "RESOLVED" task as "OPEN". #175

Closed
ftclausen opened this issue Feb 27, 2019 · 12 comments · Fixed by #188
Closed

Discussion: Marking existing "RESOLVED" task as "OPEN". #175

ftclausen opened this issue Feb 27, 2019 · 12 comments · Fixed by #188
Assignees
Milestone

Comments

@ftclausen
Copy link
Contributor

I am looking into contributing functionality to mark an existing task as "OPEN" when it's already checked (i.e. "RESOLVED" in the API). I found the Task class that, as best I can figure out, is serialised and eventually passed onto Bitbucket with the correct values. I think the serialisation + massaging field names happens in TaskApi class.

What would be ideal would be to have some setter method that can PUT the new task's status to the Bitbucket API. Given everything is being generated via Google AutoValue I'm not sure how to go about this. Just add a concrete setter to the existing AutoValue abstact Task class? That setter could directly send the appropriate JSON payload to change the task status. However that does not feel in keeping with how AutoValue is used in this class.

@cdancy
Copy link
Owner

cdancy commented Feb 28, 2019

@ftclausen so I think we'd want a new TaskApi.update method which would indeed be a PUT. As to what to pass it I'm not entirely sure at the moment. It's not clearly defined what exactly can be passed/updated. I would start with making a CreateTaskUpdate which maybe takes a handful of things the user could update. If down the road you, or someone else, wants to add additional options we could certainly do so.

https://docs.atlassian.com/bitbucket-server/rest/5.16.0/bitbucket-rest.html#idm8288367088

@cdancy
Copy link
Owner

cdancy commented Feb 28, 2019

The CreateTaskUpdate, and the API method itself, could be modeled after the create method.

@ftclausen
Copy link
Contributor Author

Thanks for the info - I'll investigate down that path and ask any follow-up questions if need be.

@ftclausen
Copy link
Contributor Author

I'm working through some options and adding a live test. However the live tests are not working for me because the endpoint seems to be null despite having updated gradle.properties to point to my local dev instance. Specifically the init step to get the endpoint is returning null - probably the authentication part too.

Do I need to do anything extra other than updating gradle.properties?

@cdancy
Copy link
Owner

cdancy commented Mar 13, 2019

Also you should need to set is testBitbucketRestEndpoint and testBitbucketRestCredentials in your gradle.properties file and you should be good to go.

@cdancy
Copy link
Owner

cdancy commented Mar 13, 2019

If it's null then something else is at play

@ftclausen
Copy link
Contributor Author

I am indeed setting those and, on an unchanged master branch, there are quite few failures when running ./gradlew integTest. They are all along the lines of:

java.lang.AssertionError: 
Expecting actual not to be null
	at com.cdancy.bitbucket.rest.TestUtilities.getDefaultUser(TestUtilities.java:87)
	at com.cdancy.bitbucket.rest.TestUtilities.initGeneratedTestContents(TestUtilities.java:190)
	at com.cdancy.bitbucket.rest.features.CommentsApiLiveTest.init(CommentsApiLiveTest.java:64)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

I'll keep digging and see what I can find.

@cdancy
Copy link
Owner

cdancy commented Mar 13, 2019

You can also, through the root build.gradle file, set just the single integTest you want to run so that you don't have to potentially sit and wait for everything.

@ftclausen
Copy link
Contributor Author

Ah, I found the issue, our username has mixed case i.e. exampleUser and com.cdancy.bitbucket.rest.domain.pullrequest.User#slug is returned as all lower case. Then, in com.cdancy.bitbucket.rest.TestUtilities#getDefaultUser there is:

            for (final User user : userPage.values()) {
                if (username.equals(user.slug())) {
                    defaultUser = user;
                    break;
                }
            }
            assertThat(defaultUser).isNotNull();

Where

username.equals(user.slug())

will never match because exampleUser != exampleuser hence the assertion triggered. If I changed that to

username.toLowerCase().equals(user.slug().toLowerCase())

then it works. I'll do some more testing to ensure nothing else breaks but, if otherwise acceptable, I can shoot in a small PR for this. Then get on with the a PR for the actual meat of this discussion.

@cdancy
Copy link
Owner

cdancy commented Mar 14, 2019

Aaaahhhh good catch ... if you just want to attach the fix to your forthcoming PR that would work as well.

@ftclausen
Copy link
Contributor Author

Another thing I had to modify was initGeneratedTestContents use the full authentication string (e.g. exampleUser:password) in the URL instead of just the username. Otherwise the integTest would ask for a password when pushing. How did you managed to get pushes to https endpoints to work with only a username? Maybe I missed something or there is a way to use ssh.

@cdancy
Copy link
Owner

cdancy commented Mar 15, 2019

I'm not sure and now you have me wondering. Add that to the PR as well and I can look things over and make sure it all still works on this end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants