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

Fixed repo permissions check #22

Merged
merged 2 commits into from
May 4, 2015
Merged

Fixed repo permissions check #22

merged 2 commits into from
May 4, 2015

Conversation

mdio
Copy link
Contributor

@mdio mdio commented May 4, 2015

While testing #15 I
Unfortunately the fix for issue #15 doesn't work correctly because when NetworkUtils.checkAvailabilityOfGitHubWithCurrentSettingsOfProject is called, success is not evaluated (see Line 121 of StatusProjectViewController.swift).
Because the error in this case is empty as well (completion(success: hasPermissions, error: nil)) the success code path is taken.

I rewrote the check in the controller to prevent this in the future (though I'm not totally happy with the code duplication for the logging).
I also added errors for all cases where success is false.

Log.error("Checking github availability error: \(error)")
} else {
Log.error("Checking github availability error: Unknown error")
}
Copy link
Member

Choose a reason for hiding this comment

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

You can do something like Log.error(error.description ?? "Unknown error") to get the same behavior without the duplication.

@czechboy0
Copy link
Member

Wow great catch! Yeah that definitely looks wrong, I was under the impression I used the hasPermission boolean, but instead I only looked at the error. I like the way you fixed it, so if you can just fix the duplication re my comment, I'll merge it right away (ignore Builda screaming that there was a build error, I just realized it can't yet do cross-fork pull requests - #23).

Thanks!

@czechboy0
Copy link
Member

Result of integration 1
Perfect build! All 7 tests passed. 👍

@czechboy0
Copy link
Member

Result of integration 2
Perfect build! All 7 tests passed. 👍

@mdio
Copy link
Contributor Author

mdio commented May 4, 2015

@czechboy0 Thanks. Learned about the nil coalescing operator. :)

@czechboy0
Copy link
Member

:) Thank you for fixing the bug. Merging.

czechboy0 pushed a commit that referenced this pull request May 4, 2015
@czechboy0 czechboy0 merged commit 53349e2 into buildasaurs:master May 4, 2015
@mdio mdio deleted the bug/fix-github-permission-check branch May 4, 2015 13:28
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