#9 notify user on new messages #26
Conversation
|
Job #26 is now in scope, role is |
|
This pull request #26 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 |
| * create a class to extract and format this information from `JsonObject` | ||
| * (github subject) accordingly. | ||
| */ | ||
| class UnreadThreads implements Notification { |
There was a problem hiding this comment.
@olenagerasimova class should be final
| * @todo #9:30 Continue implement notifications for user when new messages | ||
| * (unread github notification) arrive. Unread msgs can be retrieved with | ||
| * the help of PgThreads#unread() method, and this class is meant to sent | ||
| * them to tlg chat. So set up whole mechanism together. |
There was a problem hiding this comment.
@olenagerasimova I'm not clear what is the next step here:
- what is meant to be using
Notification? - where is
UnreadThreadsmeant to be instantiated?
Can you elaborate a bit better on what you had in mind when you introduced those abstractions?
There was a problem hiding this comment.
@victornoel Notification mission is simple - send message (in our case - to telegram chat), and UnreadThreads implementation does that - it sends telegram messages to all given users about all given github threads.
So, now it's necessary to monitor unread notification by periodically checking threads table for unread threads. Saying more practical, the whole idea is to schedule java thread (or any other synchronization process) to periodically call PgThreads#unread() to retrieve unread messages and Notification.UnreadThreads to send them.
Do you get the idea now? Or smth is still not clear?
There was a problem hiding this comment.
@olenagerasimova I do understand now, could you reword the todo to explain it?
| * @param users Users | ||
| */ | ||
| public UnreadThreads(final BotSimple bot, | ||
| final Map<Long, List<Thread>> ntfc, final Users users) { |
There was a problem hiding this comment.
@olenagerasimova why doesn't this notification directly takes PgThreads and call unreads() itself when send() is called? It would be more OO (even though I'm not sure what you had in mind).
There was a problem hiding this comment.
@victornoel I do not like the idea of passing PgThreads to this class, because PgThreads does not implement any interface. Now it's a class that depends on database source and we definitely do not want deal with it in unit tests.
There was a problem hiding this comment.
@olenagerasimova good point. Maybe @g4s8 you have an opinion if a todo should be added for introducing an interface for PgThreads or not?
There was a problem hiding this comment.
@victornoel I think we can merge this part now, but you can submit a ticket with refactoring suggestion if you see how to make it better
There was a problem hiding this comment.
@g4s8 yes, it's maybe too early to know what is the best course of action anyway :)
| * https://github.com/g4s8/teletakes/issues/14 is resolved. Please, try to | ||
| * avoid using mocked objects, use fake implementations instead. | ||
| */ | ||
| public class UnreadThreadsTest { |
There was a problem hiding this comment.
@olenagerasimova class should be final and maybe it is best to keep this todo with UnreadThreads instead of introducing this empty file.
|
@victornoel thanks for review, please, take one more look |
|
@victornoel ping |
| * | ||
| * @since 1.0 | ||
| */ | ||
| package com.g4s8.ghman.bot; |
There was a problem hiding this comment.
@olenagerasimova I believe this file is not needed anymore?
There was a problem hiding this comment.
@victornoel right now - no, but I really hope we will have tests for bot package soon and I do not see what is wrong with having empty (yet) test package. I've removed it though.
There was a problem hiding this comment.
@olenagerasimova it's better for consistency of the PR
|
@victornoel empty package removed, please, take a look |
|
@olenagerasimova thanks @g4s8 I think it's ok now |
|
@rultor merge |
|
Job #26 is not in the agenda of @victornoel/z, can't inspect |
|
@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 #26 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 |
|
@olenagerasimova the puzzle #74 is still not solved. |
For #9:
Userto get user telegram id