Skip to content

Conversation

xmedr
Copy link

@xmedr xmedr commented Jun 26, 2023

Overview

This takes the same idea of our Salsa signup/login process here and adjusts it for the Mailchimp api instead.

Notes

The file mailchimp.py is displaying here as being a new file, but it's just a renamed and adjusted salsa.py.

Also I'm getting an error when bringing up the container that there's No module named 'mailchimp_auth' but then it proceeds to finish bringing up the container and everything in the app works just fine. I imagine this temporary error is because of the order in which things are installed?

bga-payroll-worker    | Traceback (most recent call last):
bga-payroll-worker    |   File "/usr/local/bin/celery", line 8, in <module>
bga-payroll-worker    |     sys.exit(main())
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/celery/__main__.py", line 16, in main
bga-payroll-worker    |     _main()
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/celery/bin/celery.py", line 322, in main
bga-payroll-worker    |     cmd.execute_from_commandline(argv)
... extra lines from celery ...
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/celery/fixups/django.py", line 116, in django_setup
bga-payroll-worker    |     django.setup()
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/django/__init__.py", line 24, in setup
bga-payroll-worker    |     apps.populate(settings.INSTALLED_APPS)
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/django/apps/registry.py", line 91, in populate
bga-payroll-worker    |     app_config = AppConfig.create(entry)
bga-payroll-worker    |   File "/usr/local/lib/python3.5/site-packages/django/apps/config.py", line 90, in create
bga-payroll-worker    |     module = import_module(entry)
bga-payroll-worker    |   File "/usr/local/lib/python3.5/importlib/__init__.py", line 126, in import_module
bga-payroll-worker    |     return _bootstrap._gcd_import(name[level:], package, level)
bga-payroll-worker    |   File "<frozen importlib._bootstrap>", line 985, in _gcd_import
bga-payroll-worker    |   File "<frozen importlib._bootstrap>", line 968, in _find_and_load
bga-payroll-worker    |   File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
bga-payroll-worker    | ImportError: No module named 'mailchimp_auth'
bga-payroll-worker    | Signal handler <promise@0x4004a063d8 --> <bound method Celery._autodiscover_tasks of <Celery bga_database at 0x4004926e48>>> raised: AppRegistryNotReady("Apps aren't loaded yet.",)

Testing Instructions

  • Set this up with the mailchimp branch of bga-payroll:
    • Follow directions found in that repo's readme if needed
    • Add the local directory path for this repo to the app volumes in docker-compose.yml in the format - <your path>:/django-salsa-auth
    • Bring up the payroll container
  • Confirm that the following processes work as expected:
    • Logging in with a non-subscribed user and getting an error message displayed
    • Signing up and subscribing a new user
      • Before going through the email verification, it may be worth trying to log in with that user to confirm that the "We've already sent an email..." message displays as expected
    • Logging in with a subscribed user and searching

@xmedr xmedr marked this pull request as ready for review June 26, 2023 21:16
@xmedr xmedr requested a review from fgregg June 26, 2023 21:16
@xmedr
Copy link
Author

xmedr commented Jun 26, 2023

Hm, a similar error is happening during the tests for the payroll PR. I'm seeing that same error in my terminal, but it's the same weirdness where locally it just works afterwards.

What do you think @fgregg?

Copy link
Collaborator

@fgregg fgregg left a comment

Choose a reason for hiding this comment

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

a few changes to make here.

client = MailchimpAPI()


# SAMPLE_PUT_RESPONSE = json.dumps({
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's remove this commented out code

return None


client = MailchimpAPI()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we don't need this line.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha. I think this initialized client is what gets imported into views. Without it, if I do something like

from mailchimp_auth.mailchimp import MailchimpAPI as mailchimp_client

instead, it looks like I have to supply mailchimp_client as self each time I use it.

What would be the workaround here? I could instantiate it at the top of views instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you could do

from mailchimp_auth.mailchimp import MailchimpAPI

and then everywhere where you currently have mailchimp_client you could do something like

mailchimp_user = MailchimpAPI().get_supporter(email)

Wrapper for supporter methods:
https://mailchimp.com/developer/marketing/api/list-members/
'''
LIST_ID = settings.MAILCHIMP_LIST_ID
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

subscriber = payload["email_address"]

try:
client = MailchimpMarketing.Client()
Copy link
Collaborator

@fgregg fgregg Jun 27, 2023

Choose a reason for hiding this comment

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

hm.. what about setting up the client once in an __init__ method for this class?

response = client.searchMembers.search(query=email_address, fields=["exact_matches"])

except ApiClientError as error:
print("Error: {}".format(error.text))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be logged, not printed.

logging.warning("Error: {}".format(error.text))

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.

2 participants