#262 "Tasks" card improvements. #325
Conversation
|
@criske thank you for your Pull Request. I'll assign someone to review it soon. |
| @@ -42,10 +42,12 @@ public JsonTask(final Task task) { | |||
| super( | |||
| Json.createObjectBuilder() | |||
| .add("issueId", task.issueId()) | |||
| .add("invoiceNumber", task.toString().split(":")[1]) | |||
There was a problem hiding this comment.
@amihaiemil Not a big fan of this... Maybe we could pass a StatusTask to JsonTask instead,
which in return would have an invoiceId() and a status() method.
|
@amihaiemil please review this Pull Request. Deadline (when it should be merged or closed) is You should check if the requirements have been implemented (partially or in full), if there are unit tests covering the changes and if the CI build passes. Feel free to reject the PR or ask for changes if it's too big or not clear enough. Estimation here is |
|
@amihaiemil Nvm: the task value for the closed one is ok, just checked the comission is in euro not in percent and is not added to the value. |
- fix that will prevent contributor tasks from dashboard to crash
| * @version $Id$ | ||
| * @since 0.0.1 | ||
| */ | ||
| class AllTasks implements Tasks { |
There was a problem hiding this comment.
@criske Why do we need this class, what does it do exactly? :D
Also, can you write a puzzle for adding unit tests for it, if we need it?
There was a problem hiding this comment.
@amihaiemil First of all this class is needed to JsonTasks, since it accepts only Tasks.
Then this class makes possible to list all contract invoiced tasks (closed taks) along with the active tasks (see the iterator implementation).
But the requirement was to show a task being active or closed with invoice id too. This is where StatusTask decorator comes. In iterator the active tasks are mapped to a StatusTask.Active impl where invoiced tasks are mapped to a StatusTask.Closed impl.
To display a task as active or closed(with invoice) I've just overriden toString() of StatusTask impls following this convention: active:- or closed:<invoiceId>, which is kinda hacky imo :D . And finally these are parsed by JsonTask if task is StatusTask.
And about testing, I think I'll test the whole tasks endpoint. I don't see any test targeting it.
|
@criske it looks nice, but same as the other PR with big table changes, let's postpone the merge a little bit. |
|
@charlesmike deregister |
@amihaiemil ok, I've removed this task from scope. I'm not managing it anymore. |
- renamed `AllTasks` to `StatusTasks` - `StatusTasks` scales better and has 3 convenient factory methods: `all()`, `active()` and `closed()`. - Improved abstraction for `StatusTasks.StatusTask`: it has now `status()` and `invoiceId()` methods and thus making `JsonTask` to handle `StatusTasks.StatusTask` better. - units tests for `JsonTask` handling `StatusTasks.StatusTask`. - units tests for `ContractsApi#tasks` endpoint.
|
@amihaiemil I'll add tests for |
Fixes #262