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

Enable Telegram notifications for order takers #244

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

Reckless-Satoshi
Copy link
Collaborator

@Reckless-Satoshi Reckless-Satoshi commented Sep 14, 2022

Feature request, allow order takers to enable Telegram Notifications as well. It is a bad privacy trade-off for the functionality it adds, but hey, it keeps getting requested.

In addition, we move the "Enable Telegram" button to the robot profile to improve UX.

  • Create EnableTelegramDialog functional component.
  • Add Telegram messages.py for takers.
  • Dual message (maker and taker) on every call.
  • Serve Telegram activation token with /info/ if user is logged in.
  • Move Telegram Activation button from the published order stage on the TradeBox component to the Profile Dialog (watch out! A Dialog that opens a Dialog, things can get weird...)

else:
text = f"Hey {user.username}, I will send you a message when someone takes your order with ID {str(order.id)}."
text = f"Hey {user.username}, I will send you notifications about your RoboSats orders."
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not for this PR but probably it's worth it to include also i18n on the backend https://github.com/danhper/python-i18n

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, agree. There is no internationalization implemented on the backend yet, but would be very useful for the Telegram messages.

<DialogTitle id='open-dispute-dialog-title'>{t('Enable TG Notifications')}</DialogTitle>
<DialogContent>
<div style={{ textAlign: 'center' }}>
<QRCode
Copy link
Member

Choose a reason for hiding this comment

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

Is it not possible to also include on click a redirection to bot's web page same as below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean open the t.me website also on click on the <QRcode />? Sounds good!

@@ -1409,7 +1335,7 @@ class TradeBox extends Component {
enterTouchDelay={0}
title={
<Trans i18nKey='open_dispute'>
To open a dispute you need to wait{' '}
To open a dispute you need to wait
Copy link
Member

Choose a reason for hiding this comment

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

Not a translation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a translation, in this case we <Trans /> component instead of t(). We use the <Trans /> here because this translation has a children component <Countdown...>.

When running npm run format a few days ago the {' '} was introduced, I am not sure why. It made the <Countdown> component dissapear from the translations, as they were tagged with <1></1> (marks where to render the <Coundown/> in the translated component) moved to <2></2> and instead render the empty string ' '.
https://github.com/Reckless-Satoshi/robosats/blob/cd8b607891a786c77986ffee45dbd75f51610a01/frontend/static/locales/en.json#L461

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(sorry for mixing this bug fix in this PR... totally unrelated, it was bugging me!)

@@ -882,7 +880,6 @@ def cancel_order(cls, order, user, state=None):
# adds a timeout penalty
cls.cancel_bond(order.taker_bond)
cls.kick_taker(order)
# send_message.delay(order.id,'taker_canceled_b4bond') # too spammy
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of leaving commented code, do you want to just check how it goes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree,

I had commented out this line long time ago. I was not sure the solution we found was good enough. But now that I revisited this file, I think it's clear the solution worked out and it is time to delete this commented code, forever (actually, this PR also deletes the function this line is calling to) :D

@Reckless-Satoshi
Copy link
Collaborator Author

Thanks for your time reviewing @KoalaSat !

@Reckless-Satoshi Reckless-Satoshi merged commit 1ba94b2 into main Sep 15, 2022
@Reckless-Satoshi Reckless-Satoshi deleted the add-telegram-for-takers branch September 15, 2022 15:44
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

2 participants