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

A start to moving TCIntercom to FastAPI #318

Merged
merged 54 commits into from
Oct 17, 2024
Merged

Conversation

DanJezeph15
Copy link
Contributor

@DanJezeph15 DanJezeph15 commented Jul 10, 2024

Moving TCIntercom over to FastAPI, brief overview of changes.

  • Docstrings and type hints everywhere
  • Swapped setup.cfg to a pyproject.toml and added ruff instead of black in the reqs.
  • Removed a lot of code that we don't use anymore and removed docker and the heroku.yml file (a lot to do with what to do when specific tags are applied, which I've spoke to the support team about and they don't use tags at all.)
  • We now validate that webhooks we receive to the Intercom callback have actually come from Intercom by validating the x-hub-signature.
  • Slightly updated the marking duplicate contacts logic to be clearer with whats going on.
  • Overrode the arq.worker so that we can add logfire to the jobs that are called.
  • Two test files, test_main.py and test_workers.py. We now have 100% coverage in the project from these tests and I also updated them all to use @mock.patch rather than monkeypatch. Additionally updated a lot of the tests so that they actually test and check for a lot more information.
  • Added raven_dsn to app settings rather than just using os.env['raven_dsn'], and also added logfire_token to send data to logfire, log_level for the level of logging which by default is set to 'INFO' and finally ic_secret so that we can validate webhooks have actually come from Intercom as we weren't doing the before.

IMPORTANT BEFORE MERGING:

One thing to note is Heroku is setup to use Docker and I've removed all that so when we merge this, we need to have detached the auto deploy that heroku has for this repo. Then we can merge and I can test deploying this on Render

Also need to add the new environment variables to the env.

Testing

  • Test following the README is easy to follow and works and that I haven't missed anything.
  • Setup logfire by creating a project on your OWN PERSONAL projects and NOT TC2. Once you create the project on logfire, it will show you how to connect the two.
  • Setup a .env file that can store your local settings for testing for things such as ic_token, ic_secret and logfire_token

Testing the web app:

  • Check that logs get sent to logfire (web span)

    • Check that the data from the callbacks is also in the logfire logs
  • Check that webhooks are received and are processed correctly

    • Check that on conversation created, if the agency is in trial or is paying for support, then we don't reply with the bot message
    • Check that on conversation created, if the agency doesn't have a support plan and aren't in their trial period, the bot replies to the user
    • Check that any other webhooks we take no action and they get logged in logfire (Adding a user tag to the conversation)
    • Test using postman a post request with an incorrect signature
    • Test using postman a post request to the intercom callback thats invalid to test an error. For this you will need to work out the signature of the body you're posting and add it to the headers with the key x-hub-signature. For example if you're posting an empty dict you'd do the following:

    in shell

    import hmac
    import hashlib
    import json
    
    secret_key = b'<YOUR IC_SECRET TOKEN IN ENV/APP SETTINGS>'
    message = json.dumps({})
    digest = hmac.new(secret_key, message, hashlib.sha1).hexdigest()
    print(digest)
    

in your headers:
key value
x-hub-signature sha1=<digest>

Testing the worker:

  • In worker.py WorkerSettings, change the hour=1 to second=1. This will make the update_duplicate_contacts job run once a minute. I'd recommend using the user with email '[email protected]' as there are a lot of users in intercom with that email. You can go to contacts and filter to see them all. What I do to test this, is in get_relevant_accounts add an if statement after getting email and if it matches '[email protected]', then add them to either the mark_dupe or keep_contacts depending on what you're testing. For example, for marking all users with that email duplicates so you can test the correct one gets marked not a duplicate.
email = contact['email']
if email == '[email protected]':
    mark_dupe_contacts.append(contact)
    continue

and for testing that all the correct users get marked as duplicates by marking everyone with that email not a duplicate:

contacts_to_update = []
for contact in recently_active:
    email = contact['email']
    if email == '[email protected]':
        contacts_to_update.append(contact)
        continue
 ...
# Rest of the code until the keep_con_list
...
keep_con_list = [v for k, v in keep_contacts.items()]
keep_con_list.extend(contacts_to_update)
return mark_dupe_contacts, keep_con_list
  • Test that if two users with the same details exist and both are marked as is_duplicate: False then the older one gets marked as is_duplicate: True.
  • Test that if both users are marked as is_duplicate: True that the correct one is marked as is_duplicate: False
  • Check that logs get sent to logfire (worker span)
    • Check that any logs appear under the job name

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0bcca49) to head (8c93bcc).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##           master      #318       +/-   ##
============================================
+ Coverage   48.42%   100.00%   +51.57%     
============================================
  Files           7         8        +1     
  Lines         285       271       -14     
============================================
+ Hits          138       271      +133     
+ Misses        147         0      -147     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DanJezeph15 DanJezeph15 self-assigned this Jul 23, 2024
@DanJezeph15 DanJezeph15 added the enhancement New feature or request label Jul 23, 2024
@DanJezeph15 DanJezeph15 assigned Pager07 and unassigned DanJezeph15 Aug 1, 2024
@PrenSJ2 PrenSJ2 self-assigned this Aug 1, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tcintercom/app/views.py Outdated Show resolved Hide resolved
tcintercom/app/views.py Outdated Show resolved Hide resolved
tcintercom/app/views.py Outdated Show resolved Hide resolved
@DanJezeph15 DanJezeph15 assigned DanJezeph15 and unassigned Pager07 and PrenSJ2 Sep 19, 2024
@DanJezeph15 DanJezeph15 merged commit f5444b3 into master Oct 17, 2024
2 checks passed
@DanJezeph15 DanJezeph15 deleted the tc-intercom-fastapi branch October 17, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants