-
Notifications
You must be signed in to change notification settings - Fork 36
Sprint2__Subscriber Features 2 of 2 (Client dependent) #44
base: master
Are you sure you want to change the base?
Sprint2__Subscriber Features 2 of 2 (Client dependent) #44
Conversation
Negative/Formated Transaction IDs for Usage Events Block/Unblock Subscriber on Invalid Events
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. |
There was a problem hiding this 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') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 ' % ( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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!') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@sagarsharma-aricent updated the pull request - view changes |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Please review Subscriber features which include:
--> negative (for invalid events) and formatted transaction IDs for Usage Events
--> block/unblock Subscriber on Invalid Events