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

♻️ [#2257] Fetch subscriptions for user directly from Laposta #1138

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Apr 5, 2024

task: https://taiga.maykinmedia.nl/project/open-inwoner/task/2257

Notes:

  • Unfortunately it is not possible to retrieve all subscriptions for a given email adress in a single API call, so we have to perform an API call for each list
  • The API applies throttling based on the used API key, limiting to 30 calls per minute (or 120 if it's a paid account), so I decided to apply some caching to not let the number of request too out of hand

@stevenbal stevenbal marked this pull request as draft April 5, 2024 09:31
@stevenbal stevenbal force-pushed the feature/2257-fetch-laposta-subscription branch 2 times, most recently from ad9a120 to 690c1f8 Compare April 5, 2024 12:49
@stevenbal stevenbal marked this pull request as ready for review April 5, 2024 13:33
@stevenbal stevenbal force-pushed the feature/2257-fetch-laposta-subscription branch from d00f0db to 889b6fa Compare April 5, 2024 13:34
@alextreme
Copy link
Member

@Bartvaderkin to review, after approval can be merged

Comment on lines 97 to 99
# The API cannot deal with urlencoded email addresses that contain a `+`,
# so instead we fetch all members for a given list and check if there is a
# member with a matching email
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to retrieve everything is not ideal because there could be thousands of subscribers. Are we sure about the url encoding (quote plus etc)? It'd be weird if they can't handle name+dummy@gmail.com that people use all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it both with and without quoting the email:

alex+111111110@maykinmedia.nl and foo@bar.org are subscribed to the list:
Screenshot from 2024-04-08 11-17-59

I can directly fetch the relation for foo@bar.org:
Screenshot from 2024-04-08 11-19-01

But I cannot do that for the email with a plus:
Screenshot from 2024-04-08 11-18-11

Screenshot from 2024-04-08 11-18-45

Copy link
Contributor

Choose a reason for hiding this comment

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

Good tests, but it feels like something is wrong that we need to download the while subscriber database to look at a single user's subscriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll email Laposta to ask if they know about this issue, but in the meantime I'm not sure if there is another (easier) way to fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool, will be interesting. If it takes too long or they don't have anything we can use this solution and see.

Copy link
Member

Choose a reason for hiding this comment

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

But... this is mentioned in their API docs:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh well, I'll do that instead then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the workaround and replaced it with double quoting in 504b250

@stevenbal stevenbal requested a review from Bartvaderkin April 8, 2024 09:31
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 98.52941% with 1 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (develop@6d6d519). Click here to learn what that means.
Report is 6 commits behind head on develop.

Files Patch % Lines
src/open_inwoner/laposta/client.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #1138   +/-   ##
==========================================
  Coverage           ?   95.05%           
==========================================
  Files              ?      941           
  Lines              ?    33156           
  Branches           ?        0           
==========================================
  Hits               ?    31518           
  Misses             ?     1638           
  Partials           ?        0           

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

Comment on lines 97 to 99
# The API cannot deal with urlencoded email addresses that contain a `+`,
# so instead we fetch all members for a given list and check if there is a
# member with a matching email
Copy link
Contributor

Choose a reason for hiding this comment

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

Good tests, but it feels like something is wrong that we need to download the while subscriber database to look at a single user's subscriptions.

@stevenbal stevenbal force-pushed the feature/2257-fetch-laposta-subscription branch from e923445 to 9ade80c Compare April 8, 2024 11:45
@stevenbal stevenbal requested a review from Bartvaderkin April 8, 2024 11:46
@stevenbal stevenbal force-pushed the feature/2257-fetch-laposta-subscription branch from 9ade80c to e9c74d9 Compare April 9, 2024 07:33
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Noting I need to update this with the verified_email

@Bartvaderkin Bartvaderkin merged commit a6864de into develop Apr 11, 2024
15 checks passed
@Bartvaderkin Bartvaderkin deleted the feature/2257-fetch-laposta-subscription branch April 11, 2024 09:28
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.

5 participants