Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1271 Reply with estimation changed value. #1284

Merged
merged 2 commits into from
Nov 14, 2021
Merged

Conversation

criske
Copy link
Contributor

@criske criske commented Nov 12, 2021

  • Unignored test
  • Updated Step api to be able to receive data from the previous Step in chain.

Pr for #1271

- Unignored test
- Updated Step api to be able to receive data from the previous Step in chain.
@zoeself
Copy link
Collaborator

zoeself commented Nov 12, 2021

@criske thank you for your Pull Request. I'll assign someone to review it soon.

If this PR solves a todo from the code, please don't forget to remove it.

@criske
Copy link
Contributor Author

criske commented Nov 12, 2021

@amihaiemil I think with this aproach we can send the new estimation from IssueEstimationChanged to UpdateTaskEstimation,
since it's the next one in chain.

@zoeself
Copy link
Collaborator

zoeself commented Nov 12, 2021

@amihaiemil I couldn't find any assignee for this task. This is either because there are no contributors with role REV available or because the project does not have enough funds.

Please, make sure there is at least one available contributor with the required role and the project can afford to pay them.

@amihaiemil
Copy link
Member

@criske scuze, am uitat de API-u Event si faptu ca nu prea avem acces usor la vechea estimare in contextul de fata.

As renunta la SendReply in cazul asta si as folosi functie anonima, ca in alte cazuri, de exemplu aici. Sincer nici nu mai stiu acum la ce e folosit SendReply (vad ca in manager nu e folosit nicaieri) :D

Iar textul sa fie doar: "Estimation changed to X min at Timestamp" (poti lua noua estimare din event.issue().estimation()) Si pt text efectiv foloseste project.language(), sa fie traductibil : D

Altfel treaba asta cu derive, nu prea vad la ce am folosi-o. Poate ramane ca PoC pt viitor : D

@amihaiemil
Copy link
Member

@criske a, SendReply cred ca e folosit doar cand raspunde la un Comment, sa appenduiasca acolo si Comment-ul anterior.

Legat de asta cu derive(...) - nu cred ca e o idee buna, desi la prima vedere mi se pare sexy. Ideea ar fi ca un Step sa lucreze doar cu Event-ul, de acolo sa isi ia datele cu care lucreaza, exclusiv :D

@criske
Copy link
Contributor Author

criske commented Nov 13, 2021

@amihaiemil ah ok, pai daca e nevoie doar de noua estimare in comment atunci se simplifica lucrurile... ii dau revert si o sa folosesc un lambda in loc de SendReply.

Comment on lines +1890 to +1891
MatcherAssert.assertThat(commentCaptor.getValue(), Matchers
.startsWith("Estimation changed to ``30`` minutes at ``20"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//TODO we'll need to fix this test in 2100 :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took me a while to understand haha

@amihaiemil amihaiemil merged commit 52f2e11 into self-xdsd:master Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants