Skip to content
This repository has been archived by the owner on Sep 26, 2018. It is now read-only.

Sprint2__Subscriber Features 2 of 2 (Client dependent) #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sharma-sagar
Copy link

Please review Subscriber features which include:

--> negative (for invalid events) and formatted transaction IDs for Usage Events
--> block/unblock Subscriber on Invalid Events

Negative/Formated Transaction IDs for Usage Events
Block/Unblock Subscriber on Invalid Events
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

Copy link
Contributor

@kkroo kkroo left a comment

Choose a reason for hiding this comment

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

some comments about implementing the 24 hour event window

@@ -58,6 +58,9 @@
OUTBOUND_ACTIVITIES = (
'outside_call', 'outside_sms', 'local_call', 'local_sms',
)
# These UsageEvent events are not allowed block the Subscriber if repeated
# for 3 times
INVALID_EVENTS = ('error_call', 'error_sms')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep this list empty/commented out until we define the use cases and implement/validate the dialplan/chatplan event generation

Copy link
Author

Choose a reason for hiding this comment

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

yes, we have kept this as dummy for demo, as soon that functionality gets added we will replace with new EVENT.

max_length=255)
block_time = models.DateTimeField(null=True, blank=True)

class Meta:
Copy link
Contributor

Choose a reason for hiding this comment

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

why are there new permissions introduced here?

max_transactions = event.subscriber.network.max_failure_transaction
if subscriber_event.count == max_transactions:
subscriber.is_blocked = True
subscriber.block_reason = 'Repeated %s within 24 hours ' % (
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need block reason? Can we just list out the negative_transactions in the UI?

Copy link
Author

Choose a reason for hiding this comment

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

Block reason is kept for REPORTING purpose, and we are already listing negative transactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we need the reports, we have the usageevents too

negative_transactions_ids = subscriber_event.negative_transactions + [
event.transaction_id]
subscriber_event.count = subscriber_event.count + 1
subscriber_event.event_time = event.date
Copy link
Contributor

Choose a reason for hiding this comment

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

we should only update the event_time of the first event so you can do the 24 hour check

subscriber_event.save()

max_transactions = event.subscriber.network.max_failure_transaction
if subscriber_event.count == max_transactions:
Copy link
Contributor

Choose a reason for hiding this comment

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

24 hour check missing?

print 'Unblocking subscribers %s blocked for past 24 hours' % (
[subscriber.imsi for subscriber in subscribers], )
subscribers.update(is_blocked=False, block_time=None,
block_reason='No reason to block yet!')
Copy link
Contributor

Choose a reason for hiding this comment

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

we should also have another task to delete SubscriberInvalidEvents that are older than 24 hours? it seems like it would also need to update the event_time to the time of the 2nd invalid event in the negative actions list.

Copy link
Author

Choose a reason for hiding this comment

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

We are intentionally not deleting SubscriberInvalidEvents for reporting purpose.
Though it gets replaced after 24 hours by subscriber again if any negative transaction occur.

We are not updating 2nd invalid event time, as if we do we won't be able to calculate the 24 hour time span of the transactions for that particular subscriber.
Also if we need details of that transaction it can be captured from UsageEvent table as the transaction id remains same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its still not clear how we are blocking the subscriber only if it is a 3rd event in 24hr block. Can you perhaps explain the flow?

@facebook-github-bot
Copy link

@sagarsharma-aricent updated the pull request - view changes

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sharma-sagar sharma-sagar changed the title Sprint2__Subscriber Features 2 of 2 Sprint2__Subscriber Features 2 of 2 (Client dependent) Aug 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants