update to User interface to see an accept invitations#1514
update to User interface to see an accept invitations#1514ShalomGottesman wants to merge 4 commits into
Conversation
|
Job #1514 is now in scope, role is |
…tRepo and method declared in mock but not implemented
|
I learned that if I push more updates to the same branch that already has a pull request, then the new commits are added to that request. My apologies for making two requests into one. Next time I will open a separate branch if I want to request two commits be added. |
|
@ShalomGottesman I don't really understand, this PR looks almost the same as the initial one :D |
edmoffo
left a comment
edmoffo
left a comment
There was a problem hiding this comment.
The change adds two interface methods and an implementation on the Smart inner class, but several issues block merging. Four inline notes below cover a correctness bug in invitations(), a missing test, indentation that does not match the rest of the file, and dead body() calls on GET requests.
| coordsSet.add( | ||
| new Coordinates.Simple( | ||
| invite.getString("name"), | ||
| invite.getJsonObject("owner").getString("login") |
There was a problem hiding this comment.
Arguments are swapped. Coordinates.Simple takes (user, repo), so this should be (owner.login, name), not (name, owner.login). The current order yields coordinates whose user() returns the repo name.
| @Override | ||
| public boolean acceptInvitation(final Coordinates coords) throws IOException { | ||
| Iterator<JsonValue> iter = this.github().entry().uri().path("/user/repository_invitations").back().method(Request.GET) | ||
| .body().back() |
There was a problem hiding this comment.
body().back() on a GET adds nothing. Drop the body() call here and in invitations() above; the request is already a GET and the body builder is no longer setting any payload.
| .back(); | ||
| } | ||
|
|
||
| public Iterable<String> invitees() throws IOException { |
There was a problem hiding this comment.
No test covers RtRepo.invitees(). Add an RtRepoTest case that mocks the /invitations endpoint with MkContainer and asserts the parsed login set, otherwise the parsing of invitee.login is unverified.
| Set<String> invitees = new HashSet<String>(); | ||
| while (iter.hasNext()) { | ||
| JsonObject val = (JsonObject) iter.next(); | ||
| invitees.add(val.getJsonObject("invitee").getString("login")); |
There was a problem hiding this comment.
Indentation in this method mixes tabs and spaces. The rest of the file uses four-space indent only; please normalize to match before merge.
4 participants
This request is a correction to a previous one that had extra code and missed annotations. see #1513