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

#704 check purchase #709

Merged
merged 8 commits into from
Jan 28, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ Unidecode==1.0.22
ua-parser==0.8.0
user-agents==1.1.0
sorl-thumbnail==12.5.0
python-telegram-bot==11.1.0
https://github.com/selwin/django-user_agents/archive/master.zip
https://github.com/fidals/refarm-site/archive/0.4.27.zip
14 changes: 14 additions & 0 deletions shopelectro/report.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import typing

from django.conf import settings
from telegram import Bot


class TGReport:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid abbreviation and call it TelegramReport


def __init__(self):
self.bot = Bot(settings.TG_BOT_TOKEN)

def send(self, text: str):
for id in settings.TG_REPORT_ADDRESSEES:
self.bot.send_message(id, text)
1 change: 1 addition & 0 deletions shopelectro/selenium/pages/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@

from .category import CategoryPage
from .order import OrderPage
from .success import SuccesPage
13 changes: 13 additions & 0 deletions shopelectro/selenium/pages/success.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from shopelectro.selenium.pages import Page

from pages.models import CustomPage


class SuccesPage(Page):

@property
def path(self):
CustomPage.objects.get(slug='order-success').url

def is_success(self):
'Заказ принят' in self.driver.find_element_by_tag_name('h1').text
5 changes: 5 additions & 0 deletions shopelectro/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,3 +501,8 @@ def get_robots_content():

PRODUCTS_ON_PAGE_PC = 48
PRODUCTS_ON_PAGE_MOB = 12

TG_BOT_TOKEN = os.environ.get('TG_BOT_TOKEN')
TG_REPORT_ADDRESSEES = os.environ.get(
'TG_REPORT_ADDRESSEE', '@artemiy312,@duker33'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'TG_REPORT_ADDRESSEE', '@artemiy312,@duker33'
'TG_REPORT_ADDRESSEES', '@artemiy312,@duker33'

Copy link
Contributor

Choose a reason for hiding this comment

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

let's plz create some channel "shopelectro_health" or "shopeletro_report" and send stats there

).split(',')
40 changes: 27 additions & 13 deletions shopelectro/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

from django.conf import settings
from django.core.management import call_command
from selenium.common.exceptions import WebDriverException

from shopelectro import selenium
from shopelectro.celery import app
from shopelectro.report import TGReport
from shopelectro.models import CategoryPage
from shopelectro.management.commands._update_catalog import utils

Expand Down Expand Up @@ -61,18 +63,30 @@ def update_catalog():
collect_static()
]

# @todo #690:60m Handle errors in check_purchase.
# Report failed attempts. Schedule it in the celery beat.
# @todo #690:30m Schedule check_purchase in the celery beat.
Copy link
Contributor

@duker33 duker33 Jan 26, 2019

Choose a reason for hiding this comment

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

@artemiy312 , i already suggested asyncio instead of celery beat at the parent issue's PR:
#695 (comment)
What do you think about it?

Copy link
Contributor Author

@ArtemijRodionov ArtemijRodionov Jan 27, 2019

Choose a reason for hiding this comment

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

I believe it is bad idea:

  • Django is a synchronous framework that will interfere with asynchronous code
  • We need a stateful pipeline for message delivery to be able to save messages and process them later in case of deployment, bad internet connection, hardware problems, any other things that prevent a message processing (with celery we saved a lot of emails in such cases)
  • We already have working solution, that looks fine and aren't annoying us

Copy link
Contributor Author

@ArtemijRodionov ArtemijRodionov Jan 27, 2019

Choose a reason for hiding this comment

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

We can move into rq, huye or something else, but it seems it will be the same picture

Copy link
Contributor

@duker33 duker33 Jan 28, 2019

Choose a reason for hiding this comment

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

i'll answer just for history:

  • it's not big deal, i think. We can handle our async utils process it with our own separated module
  • we can provide safety by hands in every concrete case. In orders case we can (and we do) put them to the db synchronously and then do some dangerous async operations
  • we had problems with celery not long time ago: unsent mails, not launched celery services and so on. Problems for example: one, two, three



@app.task
def check_purchase():
driver = selenium.SiteDriver(site_url=settings.BASE_URL)
category_page = selenium.CategoryPage(driver, CategoryPage.objects.first().url)
category_page.load()
category_page.add_to_cart()

order_page = selenium.OrderPage(driver)
order_page.load()
order_page.fill_contacts()
order_page.make_order()
@app.task(
bind=True,
autoretry_for=(WebDriverException, AssertionError),
retry_kwargs={'max_retries': 3},
)
def check_purchase(self):
try:
driver = selenium.SiteDriver(site_url=settings.BASE_URL)
category_page = selenium.CategoryPage(driver, CategoryPage.objects.first().url)
category_page.load()
category_page.add_to_cart()

order_page = selenium.OrderPage(driver)
order_page.load()
order_page.fill_contacts()
order_page.make_order()

success_page = selenium.SuccessPage(driver)
success_page.load()
assert success_page.is_success()
except (WebDriverException, AssertionError) as err:
if self.request.retries + 1 > self.max_retries:
Copy link
Contributor

Choose a reason for hiding this comment

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

one failed retry seems to me enough reason to send the report. Why you choose three ones?
And we should send one report for one failing. With this code celery will spawn our telegram channel

Copy link
Contributor Author

@ArtemijRodionov ArtemijRodionov Jan 27, 2019

Choose a reason for hiding this comment

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

one failed retry seems to me enough reason to send the report

but what if it is just false positive temporary issue, that usually occurs.
just in case i will create the setting variable for retry number

With this code celery will spawn our telegram channel

It's not, it will only send the message on the last attempt.

Copy link
Contributor

Choose a reason for hiding this comment

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

@artemiy312 , but this last attempt occurs every max_retries check. It increases an interval of repeated messages, but still called spawn =)

but what if it is just false positive temporary issue, that usually occurs.

it's not good, that it occurs. Tests on CI works fine. You can create setting var of course. But this problem requires more systematic decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@duker33 ,

but this last attempt occurs every max_retries check. It increases an interval of repeated messages, but still called spawn

i don't see the point. Can you describe it in more detail?
I believe it will report an error once at the end of the retry counter.
e.g. if max_retries = 3 then the third failed try only will be reported, and previous once will be omitted

But this problem requires more systematic decision

Let's just set max retries number to 1 and if there are issues with this, we will find a solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just set max retries number to 1 and if there are issues with this, we will find a solution

@artemiy312 , y, it's perfect.

About max_retries - merge this code plz. I'll create issue and we'll discuss it separately

TGReport().send(f'Can\'t buy a product. Got the error: {err}')
raise err