Add invites/signups to Slack, PiratNyt etc to post-signup page #575
Conversation
JondareHM
requested review from
JakobLibak,
lakridserne,
nallic and
Rotendahl
as code owners
There was a problem hiding this comment.
Yo, fedt at du er kommet vider med det issue.
Egentligt skulle jeg have beskrevet fremgangsmåden lidt bedre.
For at få dem skrevet op til nyhedsbrevet skal vi helt rigtigt sende en request til mailchimp.
Dette udklip fra deres api siger hvordan vi kan tilføje dem
https://mailchimp.com/developer/guides/manage-subscribers-with-the-mailchimp-api/
Det kan vi så enten vælge at gøre via python på backenden eller via javascript i frontenden.
Begge dele har sine fordelle og ulemper.
Fordelen ved at gøre det i backenden er at vi f.eks kan sætte en bool på person modellen så vi kan se alle dem der får nyhedsbrevet. Det ved jeg ikke hvor nødvendigt er, det kan man nok se i mailchimp backenden. Vi også bedre lave fejl håndtering i backenden, det er sværere at fange et crash i brugerens browser end i vores backend. (Ihvertfald indtil vi sætter sentry på javascripten).
Fordellen ved frontend er at det er en smule mindre kode.
Du bestemmer selv hvad du føler mest for :)
At have en seperat form til det er helt fint, så skal vi bare snart have error tracking via browseren, men det er ikke så slemt at sætte op.
Koden som den er nu er der dog nogle forbedringsforslag til.
Jeg gætter på at det bare er noget kode du har fundet mailchimps egen side eller ligende?
Det er der sådan set ikke noget galt med, men der skal ændres i den før det sættes ind.
Som det er nu er koden ikke nem at forstå.
Formatering gør koden meget svær at læse, med så lange linjer.
Specielt det javascript i bunden. Split CSS og javascript over i sine egne filer og
og formater divs osv. så linjerne bliver kortere.
Der tilføjes også et eksternt stylesheet inline i koden.
(linjen)
<link href="//cdn-images.mailchimp.com/embedcode/classic-10_7.css" rel="stylesheet" type="text/css">
Hvad gør den CSS og hvorfor skal den med?
En anden ting er alt den javascript der bliver loadet.
Udover at der læses noget fra en s3 bucket i bunden så er der en jQuery reference til sidst i
det script. Jquery bliver kun loadet som en sideeffekt af bootstrap så det er lidt held at det overhovedet virker. Hvis vi en dag fjerner bootstrap så dør signup form og det er der ingen test til at tjekke for.
Koden blokken her, lyder skræmmende.
</div> <!-- real people should not fill this in and expect good things - do not remove this or risk form bot signups-->
<div style="position: absolute; left: -5000px;" aria-hidden="true"><input type="text" name="b_55ca261a7dd7f512770953b3a_1c46da61ea" tabindex="-1" value=""></div>
<div class="clear"><input type="submit" value="Subscribe" name="subscribe" id="mc-embedded-subscribe" class="button"></div>
Jeg ved ikke hvad der foregår og kan ikke gennemskue det fra context. Hvis det er noget man skal i følge deres API skal vi nok have et link eller noget til det så man kan sætte sig ind i det.
|
@Rotendahl Det er simpelthen bare en embed form som de har genereret specifikt til at man kan proppe ind og derigennem lade folk tilmelde sig. Helt klar på at det ikke ligefrem er kønt kode, men ville bare heller ikke røre for meget ved det når nu det var embed kode. Men lyder som om du vil lave noget helt fra bunden af i stedet for? |
|
@JondareHM er med på det er embedded kode, som udgangspunkt er der heller ikke noget i vejen med at tage kode direkte Når du tager ting fra nettet er det typisk skrevet til at være selvindholdt så der vil de ikke have splittet det ud på flere filer osv.
Til kode udefra er det også god stil med et link til hvor det kommer fra. Hvis andre så skal arbejde med det senere slipper de for at google rundt for at finde hvor koden kommer fra. Når vi nu ikke bruger mailchimp andre steder kan du bare sætte link inline i stedet for i contrib.md |
|
En sidste ting, det slår mig lige at hvis vi laver formen som sin egen template så kan vi også sætte den ind et andet sted på siden til dem der er logget ind. Som det er nu så er det jo kun nye opskrivninger der får muligheden for at se den. @JakobLibak har du en holdning til om vi skal vise den til allerede tilmeldte folk og hvilken sider den ville passe bedst på (log ind, oversigtssiden, min profil, eller en anden? |
|
@Rotendahl Ja det var faktisk også meningen, havde bare smidt den ind her for hurtigt at kunne komme i gang med at lege med den og finde ud af hvordan det hele virker. Men ideen er at jeg smider den i sin egen form som så bare bliver indlejret på user_created siden, og en anden "kommunikation" side sammen med alle de andre kanaler, som så er tilgængelig fra hovedsiden et eller andet sted. Men til selve formen, stadig ikke sikker på hvad du vil have mig til: Skal jeg lave noget helt fra bunden, eller bare fortsætte med at få ryddet op i det her Embed kode så det er mere læsbart osv? |
|
Super, jeg var ikke helt med på hvor meget "work in progress" det var :) Hvis vi laver den i backenden kan vi jo egenligt skjule formen hvis de allerede er tilmeldt, hvis vi kun har formen i frontend så vil den altid være der, selvom de allerede er skrevet op. Men det gør nok ikke så meget, det lader vi @JakobLibak om at bestemme. Jeg tjekkede lige hurtigt deres API, og kastede noget kode sammen som eksempel. Koden her er nok 70% færdig. Du kan også lave den i javascript med fetch api'en hvis du hellere vil det. import requests
list_id = "9e67587f52"
response = requests.request(
"POST",
f"https://api.mailchimp.com/3.0/lists/{list_id}/members/",
json={
"email_address": "[email protected]",
"status": "subscribed",
"merge_fields": {"FNAME": "Urist", "LNAME": "McVankab"},
},
auth=('cp', 'api_key'),
headers={
"content-type": "application/json",
},
)Du kan også bare gøre formen pænere og fjerne de ting som ikke behøves. |
|
Eftersom formen jo primært er til nye folk som tilmelder sig som frivillig tænker jeg ikke at der kommer til at være ret mange som allerede er skrevet på. Either way, regner med bare at arbejde videre på embed koden, får smidt det over i sin egen Form fil, og hevet alt inline CSS'en over i sin egen fil også, sammen med hvad jeg ellers laver af styling. |
…rt moving it into it's own files next.
|
Er noget klart til test lige nu? Jeg kan ikke se noget anderledes på /volunteer eller forsiden. Hvis man kan tilmelde sig nyhedsmail, så vil det være en fordel af kunne afmelde sig også herinde, så man kan administrere sin profil herfra. Og kan det laves så meget som en indbygget funktion at kan skifte væk fra MailChimp og bruge et nyt API-kald fra en nyskrevet funktion uden at skulle skrive noget front-end kode om? |
…d it. - Most of the text and the Facebook image is still temporary
…'s visible based on volunteer status
…/forenings_medlemmer into volunteer_invites_page
…/forenings_medlemmer into volunteer_invites_page
Codecov Report
@@ Coverage Diff @@
## master #575 +/- ##
==========================================
- Coverage 69.26% 69.14% -0.13%
==========================================
Files 111 112 +1
Lines 2834 2852 +18
==========================================
+ Hits 1963 1972 +9
- Misses 871 880 +9
Continue to review full report at Codecov.
|
|
Givet merge conflicts, har vi tilladt os at lave en simplere implementation i #787. Så lukker dette PR, tak for hjælpen og indsatsen 🙂 |
implements #482, #486