-
-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
Job #57 is now in scope, role is |
This pull request #57 is assigned to @victornoel/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova some comments
@@ -106,7 +107,7 @@ public void run() { | |||
new TkCloseIssue(this.users) | |||
) | |||
), | |||
new FbUnauthorized() | |||
new FbUnauthorized(new EnvironmentVariables()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova BotApp
should be the one taking an EnvironmentVariables
object and passing it to FbUnauthorized
as well as other objects if needed.
* @since 1.0 | ||
* @checkstyle ClassDataAbstractionCouplingCheck (500 lines) | ||
*/ | ||
class FbUnauthorizedTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova class must be final
"Returns link to login", | ||
new FbUnauthorized( | ||
new EnvironmentVariables(new MapOf<>(new MapEntry<>("APP_HOST", host))) | ||
).handle(null, new GhAuthException(new User.Fake(new MkGithub()), "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova pass a simple new Update()
instead of null
void returnsEmptyResponse() { | ||
new Assertion<>( | ||
"Returns empty response", | ||
new FbUnauthorized(new EnvironmentVariables()).handle(null, new Throwable()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova pass a simple new Update()
instead of null
.orElseThrow( | ||
() -> new IllegalStateException("Response is not supposed to be empty!") | ||
).xml(), | ||
XhtmlMatchers.hasXPath( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova if possible replace this static call with an OO solution. For example by composing new MatcherOf
and the use of xembly for testing the content of the xml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel check this comment
} | ||
|
||
@Test | ||
void returnsEmptyResponse() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olenagerasimova can we rename this to something like 'ignoresNonGhAuthException` or similar to better explicit the intent (maybe I misunderstood the intent though)?
@victornoel done |
@g4s8 same question as in #58 (comment) about |
@victornoel/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
@g4s8 ping |
1 similar comment
@g4s8 ping |
@rultor merge |
@ammaratef45/z please review this job completed by @victornoel/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
The job #57 is now out of scope |
@0crat quality good |
Order was finished, quality is "good": +20 point(s) just awarded to @victornoel/z |
Quality review completed: +4 point(s) just awarded to @ammaratef45/z |
For #37
EnvironmentVariables
toFbUnauthorized