Skip to content

Conversation

@mlegore
Copy link
Contributor

@mlegore mlegore commented May 30, 2018

Purpose:

  • Adds support to get session level decisions.

Technical overview:

  • Updates client to call additional endpoint with session_id

Testing Plan

  • Expanded and added unit tests and run them locally.

Deployment

  • Publish to pip
  • Github release

Related: https://github.com/SiftScience/code/issues/25317

Added API client method for calling user session get decisions API
described here:
https://siftscience.com/developers/docs/curl/decisions-api/decision-status
for SiftScience/code#25317
@mlegore mlegore requested review from garylee1 and jintaekim20 May 30, 2018 23:17
sift/client.py Outdated
Args:
user_id: The ID of a user.
session_id: The ID of a session

Choose a reason for hiding this comment

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

nit: . is missing (for the sake of consistency)


def test_get_session_decisions(self):
mock_response = mock.Mock()
mock_response.content = '{"decisions":{"payment_abuse":{"decision":{"id":"user_decision"},"time":1468707128659,"webhook_succeeded":false}}}'
Copy link

@jintaekim20 jintaekim20 May 31, 2018

Choose a reason for hiding this comment

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

More realistic example will be account_takeover instead of payment_abuse. It will be also better if we have session_decision as an ID, as we're adding a function to query session decisions.

As for the examples in the real world, I'll sync up with you on how we can use api3 client tool.

Copy link

@jintaekim20 jintaekim20 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Left a couple of comments. Please make sure to add an example in README.md, as well. cc @mjouahri


def test_get_session_decisions(self):
mock_response = mock.Mock()
mock_response.content = '{"decisions":{"account_abuse": {"decision": {"id": "session_decision"},"time": 1461963839151,"webhook_succeeded": true}}}'

Choose a reason for hiding this comment

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

I meant account_takeover here because session decision is only available for ATO, though it doesn't affect the test result itself.

Copy link

@jintaekim20 jintaekim20 left a comment

Choose a reason for hiding this comment

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

I left another comment regarding mock response. Otherwise, LGTM.

@mlegore mlegore merged commit c2ab116 into master Jun 1, 2018
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.

3 participants