Skip to content
This repository has been archived by the owner on Jun 29, 2021. It is now read-only.

#554 - "Remove branch" button is not displayed as expected #559

Merged
merged 9 commits into from
Sep 6, 2016

Conversation

LetItRock
Copy link
Contributor

Changed show "Delete branch" button logic:

  • pull request state should be closed or merged
  • user must have push permissions to head repository
  • head reference should exist

I have tested this change and its working for me... but it would be better if you @alorma will do some testing also...

Pavlo Tymchuk and others added 5 commits September 2, 2016 18:35
- created separate reference service
- created get/patch/delete reference service clients
- added delete branch button to pull request detail issue view
- implemented delete branch button logic

-- need to fix some little issue with calls to server: view is created before check branch exist call
- resolved problem with refreshing pull request header item (previous commit)
- created GitReferenceService.HEADS string which should be added to every reference request
- after pull request data is received -> check if head branch exist; this information is showing for us when to display delete branch button
- changing color for delete button - red when pull request is closed but not merged, purple when it is closed and merged
  1. If head repository is not fork and user has push permissions
  2. If user has push permissions to head repository

- PullRequestConversationFragment - request to get head repository info - permissions
- flatMap for two requests - repository info and reference info
- check if user has push permissions to head branch and it exists
# Conflicts:
#	app/src/main/java/com/alorma/github/ui/fragment/pullrequest/PullRequestConversationFragment.java
#	app/src/main/java/com/alorma/github/ui/view/pullrequest/PullRequestDetailView.java
@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

@juherr ping!

.observable()
.flatMap((repo) -> {
hasPushPermissionsToHead = repo.permissions.push;
Copy link
Contributor

Choose a reason for hiding this comment

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

should check if permissions is not null, i get null sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok didn't know about that, I will fix this...

@juherr
Copy link

juherr commented Sep 6, 2016

If you provide an apk, I will test it on my side.

@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

👍

Approved with PullApprove

- changes related to pull request - CI
@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

Great! If you could generate an APK, and send it to @juherr , he can test :P

@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

Merge the latest change from develop plz

@LetItRock
Copy link
Contributor Author

Ok. @juherr I have sent APK to you :) Please check your email box julien@herr.fr

- fix for NullPointerException
@juherr
Copy link

juherr commented Sep 6, 2016

👍 As replied by email, let me few days to test it because I don't have any merged pull-request.

@LetItRock
Copy link
Contributor Author

LetItRock commented Sep 6, 2016

You can create pull request for my repo changing README.md
https://github.com/LetItRock/test
And I will merge it...

@juherr
Copy link

juherr commented Sep 6, 2016

Nice try but: "Failed to delete branch" on both refused and merged branches.
Any way to activate logs?

@LetItRock
Copy link
Contributor Author

That means that we get different response from 204...
https://developer.github.com/v3/git/refs/

@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

If you tell me where to test, i will test it later

@LetItRock
Copy link
Contributor Author

@juherr could you check it again with latest APK I have sent?

@juherr
Copy link

juherr commented Sep 6, 2016

It works now ! Congratulations

@alorma alorma merged commit 7f12642 into gitskarios:develop Sep 6, 2016
@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

Merged!

@LetItRock
Copy link
Contributor Author

Great! :) Next time I should be more precise with testing...

@alorma
Copy link
Contributor

alorma commented Sep 6, 2016

Doing those PRs was awesome!

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

Successfully merging this pull request may close these issues.

None yet

3 participants