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

73 #87

Closed
wants to merge 15 commits into from
Closed

73 #87

wants to merge 15 commits into from

Conversation

l3r8yJ
Copy link
Collaborator

@l3r8yJ l3r8yJ commented Sep 9, 2023

closes #73


PR-Codex overview

This PR focuses on making several changes to the codebase. The notable changes include:

  • Deleting the Registry.java and RegistryTest.java files
  • Modifying the answers.properties file to update the token and registry properties
  • Adding dependencies for cactoos and easy-random-core in the pom.xml file
  • Modifying the Start.java file to use Mono from Reactor
  • Adding a license header to the package-info.java file
  • Modifying the Conversation.java interface to use Mono from Reactor
  • Adding the Unsubscribe.java file to handle the /chatid command
  • Adding a license header to the TokenOf.java file
  • Adding a test case for the TokenOf class

The following files were skipped due to too many changes: src/test/java/io/blamer/bot/text/TokenOfTest.java, src/main/java/io/blamer/bot/agents/tg/TgBot.java, src/test/java/extension/UpdateWithTokenExtension.java, src/main/java/io/blamer/bot/conversation/routes/Token.java, src/test/java/io/blamer/bot/conversation/routes/TokenTest.java, src/test/java/io/blamer/bot/agents/tg/TgBotTest.java

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@zoeself
Copy link

zoeself commented Sep 9, 2023

@l3r8yJ 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.

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Patch coverage: 46.15% and project coverage change: -23.25% ⚠️

Comparison is base (b626110) 80.89% compared to head (bc915d5) 57.65%.
Report is 2 commits behind head on master.

❗ Current head bc915d5 differs from pull request most recent head 6f1a3c0. Consider uploading reports for the commit 6f1a3c0 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             master      #87       +/-   ##
=============================================
- Coverage     80.89%   57.65%   -23.25%     
  Complexity       32       32               
=============================================
  Files            10       12        +2     
  Lines            89      111       +22     
  Branches          2        1        -1     
=============================================
- Hits             72       64        -8     
- Misses           17       47       +30     
Files Changed Coverage Δ
...va/io/blamer/bot/conversation/routes/Registry.java 20.00% <0.00%> (-74.45%) ⬇️
...io/blamer/bot/conversation/routes/Unsubscribe.java 20.00% <20.00%> (ø)
src/main/java/io/blamer/bot/agents/tg/TgBot.java 66.66% <35.71%> (-28.34%) ⬇️
.../java/io/blamer/bot/conversation/routes/Start.java 100.00% <100.00%> (ø)
.../java/io/blamer/bot/conversation/routes/Token.java 100.00% <100.00%> (ø)
src/main/java/io/blamer/bot/text/TokenOf.java 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoeself
Copy link

zoeself commented Sep 9, 2023

@h1alexbel please review this Pull Request. Deadline (when it should be merged or closed) is 2023-09-12T20:18:59.270984.

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 30 minutes, that's how much you will be paid. You will be paid even if this PR gets rejected.

Copy link
Member

@h1alexbel h1alexbel left a comment

Choose a reason for hiding this comment

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

@l3r8yJ take a look at my comments, please

src/main/java/io/blamer/bot/conversation/Conversation.java Outdated Show resolved Hide resolved
* Chat id command.
*/
@Component("/chatid")
public class ChatId implements Conversation {
Copy link
Member

Choose a reason for hiding this comment

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

@l3r8yJ maybe a better name for this class will be RegisterChat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h1alexbel Honestly, I'd rather make this command like /unsubscribe because we already have /register

Copy link
Member

Choose a reason for hiding this comment

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

@l3r8yJ we don't have register chat id command.
We have registry, but it's a different flow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h1alexbel I think you can get by with a registry, storing the chat id + token at the same time

Copy link
Member

Choose a reason for hiding this comment

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

@l3r8yJ you can, but still now we have a separate flow for it, simply because register chat is like user registration in blamer, while register token is actually different flow

What do you think?

Copy link
Collaborator Author

@l3r8yJ l3r8yJ Sep 11, 2023

Choose a reason for hiding this comment

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

@h1alexbel honestly, I don't see any cases where a user would want to save a chat without receiving notifications

what's the motivation behind it?

Copy link
Member

Choose a reason for hiding this comment

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

@l3r8yJ actually yes, agree.
So we will reduce amount of commands simply to: /unsub and /token. On token we will validate if the chat already exists in db

Copy link
Member

@h1alexbel h1alexbel Sep 11, 2023

Choose a reason for hiding this comment

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

@l3r8yJ can you apply these changes to diagrams in order to sync up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h1alexbel i'll create a puzzle

Copy link
Member

Choose a reason for hiding this comment

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

Ok

src/main/java/io/blamer/bot/text/TokenOf.java Show resolved Hide resolved
src/main/resources/answers.properties Outdated Show resolved Hide resolved
src/test/java/extension/UpdateWithTokenExtension.java Outdated Show resolved Hide resolved
Иван Иванчук added 3 commits September 10, 2023 19:41
@l3r8yJ l3r8yJ requested a review from h1alexbel September 10, 2023 16:57
@l3r8yJ
Copy link
Collaborator Author

l3r8yJ commented Sep 10, 2023

@h1alexbel take a look, please

@zoeself
Copy link

zoeself commented Sep 11, 2023

@h1alexbel Don't forget to close this ticket before the deadline (2023-09-12T20:18:59). You are past the first half of the allowed period.

Иван Иванчук added 2 commits September 11, 2023 10:49
@l3r8yJ l3r8yJ requested a review from h1alexbel September 11, 2023 08:00
@l3r8yJ
Copy link
Collaborator Author

l3r8yJ commented Sep 11, 2023

@h1alexbel take a look, please

@h1alexbel
Copy link
Member

@l3r8yJ commented on mixing flows

@h1alexbel
Copy link
Member

@l3r8yJ here is new version #88

@h1alexbel h1alexbel closed this Sep 12, 2023
@zoeself
Copy link

zoeself commented Sep 12, 2023

@h1alexbel thank you for resolving this ticket. I've just added it to your active invoice. You can always check all your invoices and more on the Contributor Dashboard.

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.

/chat-id telegram bot command
3 participants