Closed toretefre closed 2 years ago
Visit the preview URL for this PR (updated for commit 014e453):
https://atb-webshop-staging--pr449-tefre-dynamic-suppor-nncjatge.web.app
(expires Sun, 09 Jan 2022 15:46:12 GMT)
🔥 via Firebase Hosting GitHub Action 🌎
Kunne vi samtidig fikset slik at denne støtter at bruker er innlogget med epost og ikke har registrert telefon? Dette ble nok implementert mens vi bare hadde telefon-innlogging, og ikke endret etter vi la til epost. Det er ikke sikkert at epost bruker sender denne mailen fra samsvarer med den som er brukt under registreringen.
Ja, kan se på det!
@toretefre jeg så ikke på dette før jul. Kan du ta det?
Kan gjøre et forsøk! @mikaelbr
Her tenker du bare på innholdet i e-posten, @gorandalum? La nå inn at både telefonnummer og e-post inkluderes hvis de finnes. Den defaulten som eksisterte fra før (<Telefonnummer her>
) ser ikke ut til å brukes for tiden, men inkluderte en default for e-post likevel i tilfelle den skulle bli aktuell igjen.
Jeg ville nok i teksten vært mer eksplisitt på hva som faktisk er innloggingsmetoden til brukeren.
Forslag til innledning:
Jeg ønsker at min profil med all tilhørende informasjon slettes fra nettbutikk og AtB-systemene. Profilen min er tilknyttet telefonnummer: 99999999
eller
Jeg ønsker at min profil med all tilhørende informasjon slettes fra nettbutikk og AtB-systemene. Profilen min er tilknyttet e-post: test@test.no
Hva som skal brukes blir da basert på signInMethods
i profile
. Det er en teknikalitet rundt at e-post ikke er garantert unik mellom kontoer som gjør at dette blir mer riktig.
Da skal ting fungere som nevnt i siste kommentar her, med separat tekst for telefon / e-post. Har ikke tatt hensyn til at det muligens finnes situasjoner der det finnes kontoer uten signinProviders, men da skal det egentlig ikke være mulig å logge inn, eller bruker vil oppleve mange andre problemer.
Ser det greit ut, @gorandalum ?
Dette fungerer med hvordan ting fungerer i praksis nå, så er det et spørsmål hvor nøye man skal være for å unngå feil i fremtiden. Datamodellen med signInMethods
støtter at man kan ha flere innloggingsmetoder på en konto. Dette er også noe som er ønskelig at blir mulig i praksis, ved at registrering av mobilnummer på en konto med innloggingsmetode epost/pw gjør at man kan logge inn på den kontoen også med det registrerte nummeret.
Om/når det blir mulig med flere innloggingsmuligheter så er det fare for at det oppstår feil med som er nå. Verdien til hasPhoneProvider
er basert på om Phone
er i signInMethods
, men identificator
hentes fra det første innslaget i signInMethods
. Dette vil kunne gi bugs i fremtiden om det første innslaget i signInMethods
er epost, og det andre innslaget mobilnummer. Jeg tror det lønner seg å investere noen minutter på å fikse det nå for å hindre bugs i fremtiden.
@toretefre Tenker det er bedre med en slik løsning som vi diskuterte tidligere hvor det bare sjekker om det er telefon og viser det om mulig om ikke vis epost. Eventuelt mappe over en case fra Provider.
@gorandalum Som jeg sa til Tore tidligere da, vi er jo egentlig ikke avhengig av å vise alle eller "første innlogging" osv her. Det som er det reelle behovet er å finne en verdi som er søkbar i systemet (og spesifikt Sørvis da). Vi velger det her som signInProvider for datastrukturmessig har vi spesifisert at det er den eneste verdien vi er "garantert" til å ha som er søkbar i Sørvis (det kan være tilfeller hvor det ikke er sant om f.eks lagring av Entur profil har feilet, men da har vi også mange andre problemer og det er noe som må fikses uavhengig av dette som vi har diskutert flere ganger før).
Tusen takk for god feedback!
Nå har jeg oppdatert slik at vi foretrekker telefonnummer, men viktigst av alt kommer identifier
nå fra en matchende oppføring, i de tilfellene både telefon og e-post er registrert som signinMethods
. Det hadde nok ikke blitt et problem ettersom vi, som @mikaelbr nevner, egentlig bare trenger en identifikator som er søkbar i Sørvis, men jeg er helt enig i at vi gjerne kan gjøre et forsøk på å forhindre fremtidige bugs.
Skilte forresten også logikken for å hente identifier
ut til en egen funksjon, getIdentifier
.
Forespørsel om sletting av bruker i nettbutikk går i dag over e-post til kundeservice i AtB. OOS-aktørene vil nok bruke egen kundeservice til dette, og derfor bør e-postadressen her være dynamisk. Dette løses i denne PR.
Eventuelle andre referanser til kundeservice burde også være dynamiske, men jeg klarte ikke finne noen.