-
Notifications
You must be signed in to change notification settings - Fork 6
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
✨ [#2303] Improvements/fixes for Laposta #1170
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1170 +/- ##
===========================================
+ Coverage 95.13% 95.18% +0.04%
===========================================
Files 957 956 -1
Lines 34645 34603 -42
===========================================
- Hits 32960 32937 -23
+ Misses 1685 1666 -19 ☔ View full report in Codecov by Sentry. |
181d305
to
b86f8c3
Compare
1e5213d
to
2564775
Compare
|
||
list_name_mapping = dict(self.fields["newsletters"].choices) | ||
user_data = UserData( | ||
ip=get_client_ip(request)[0], | ||
email=user.verified_email, | ||
source_url=None, | ||
custom_fields=None, | ||
custom_fields={"toestemming": "Ja, ik wil de nieuwsbrief ontvangen"}, |
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.
this is actually the expected value, instead of True
.
@alextreme the custom_fields
are configurable in Laposta, do we want this to be dynamic from our end as well?
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.
@stevenbal ugh. Yes, please make this configurable on our end. Then we can use the same text for asking permission.
Are you sure this is the same value for all newsletters? This feels like something which is newsletter-specific...
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.
From what I saw, it seems to be the same for all lists (I would have to double check), but it is configurable per list so it's technically possible that they're different
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.
Split off my request into https://taiga.maykinmedia.nl/project/open-inwoner/issue/2413 , it's out of scope
Fixes for laposta integration after testing done in #2303
Changes:
toestemming
field