#38 TkNotifications test #58
Conversation
|
Job #58 is now in scope, role is |
|
This pull request #58 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 |
| @@ -64,7 +62,7 @@ public TmResponse act(final Update update) throws IOException { | |||
| ntf -> Collections.singleton( | |||
| new MapEntry<>( | |||
| ntf.subject().getString("title"), | |||
| new FormattedText("click:notification#%s", ntf.tid()).toString() | |||
| new FormattedText("click:notification#%s", ntf.tid()).asString() | |||
There was a problem hiding this comment.
@olenagerasimova good find, for the record this change will become useless once #40 is solved, but I prefer the usage of asString() like you did when possible :)
There was a problem hiding this comment.
@victornoel I had to correct it, test can not work otherwise
There was a problem hiding this comment.
@olenagerasimova I know, I was just sharing information :)
| import javax.json.JsonObject; | ||
|
|
||
| /** | ||
| * Implementation of {@link Github} that to returns given in ctor request body. |
There was a problem hiding this comment.
@olenagerasimova the sentence is not correct grammatically I think
| Json.createArrayBuilder() | ||
| .add( | ||
| Json.createObjectBuilder().add(id, idone) | ||
| .add(subject, Json.createObjectBuilder().add(title, subjone)) |
There was a problem hiding this comment.
@olenagerasimova put one add (one for id, one for subject) per line for better readibility
| ) | ||
| .add( | ||
| Json.createObjectBuilder().add(id, idtwo) | ||
| .add(subject, Json.createObjectBuilder().add(title, subjtwo)) |
There was a problem hiding this comment.
@olenagerasimova put one add (one for id, one for subject) per line for better readibility
| ) | ||
| ) | ||
| ).act(upd).xml(), | ||
| XhtmlMatchers.hasXPaths( |
There was a problem hiding this comment.
@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.
@victornoel I think that making code not obvious and more complicated than it could be in order to avoid using static method of the external library is greater evil then straightforward and obvious call of this static method. @g4s8 wdyt?
There was a problem hiding this comment.
@olenagerasimova I agree, I think we have a problem here that I think in the long run we need to fix, but it's outside of this issue scope.
|
@victornoel fix and comment |
|
@g4s8 we have a problem because there is no other tool than Apart from that I'm good to merge. |
|
@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 |
|
@g4s8 ping :) |
|
@victornoel thanks |
|
@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 |
|
Job #58 is not in the agenda of @victornoel/z, can't inspect |
|
The job #58 is now out of scope |
|
@0crat quality good |
|
Quality review completed: +4 point(s) just awarded to @ammaratef45/z |
|
Order was finished, quality is "good": +20 point(s) just awarded to @victornoel/z |
For #38
FormattedTextusage inTkNotificationsRequestGithubto make testing possible