-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: Transmit correct account identifiers on reconnect (SCR-508) #2528
Conversation
@@ -99,7 +99,7 @@ export class DumbTriggerManager extends Component { | |||
|
|||
async handleSubmit(data = {}) { | |||
const { client, flow, konnector, vaultClient, t } = this.props | |||
const { account } = this.state | |||
const account = this.state?.account || flow.account |
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.
state
est toujours defined non ? 🤔
Est-ce que ne ferait pas mieux de laisser const { account } = this.state
ici et de faire plutôt un const account = props.account || props.flow.account
à la place de const { account } = props
dans le constructor ?
Mais du coup ça pose question, pourquoi avoir un account et un flow.account dans les props ? 🤔 D'ailleurs ça parait legitime d'après le descriptif des propTypes d'avoir un account undefined, c'est donc étonnant d'avoir un account undefined et un flow.account defined, non ? 🤔
Dans les propTypes on pourrait rajouter de la doc concernant la prop flow d'ailleurs
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.
state est toujours defined non ?
Oui, c'est corrigé
c'est donc étonnant d'avoir un account undefined et un flow.account defined, non ?
On a le cas account undefined
et flow.account
defined justement quand l'utilisateur a fait une première tentative (-> donc account et trigger créé) et qu'il refait une nouvelle tentative en restant sur le même écran de login
Est-ce que ne ferait pas mieux de laisser ...
Oui c'est probablement mieux et moins enfoui au milieux du code comme ça, corrigé
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.
@doubleface si tu peux rajouter un petit commentaire quelque part ou de la doc pour expliquer dans le code comment on peut se retrouver dans le cas account undefined
et flow.account defined
(comme on vient de le faire ici en commentaire) c'est top 👍
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.
je valide la PR pour déblocage, à merger modulo les réponses aux questions posées précédemment
e2ba8cd
to
77141c9
Compare
When the user tries to connecteur a server konnector with wrong identifiers and fix identifiers and retry conection, the previous identifier a resent to the konnector and an io.cozy.account document, not linked to any trigger is also created for each atempt. Now the trigger manager will reuse the account from the the current ConnectionFlow instance when possible.
77141c9
to
f4394a9
Compare
To transmit correct account identifiers on reconnect cozy/cozy-libs#2528
To transmit correct account identifiers on reconnect cozy/cozy-libs#2528
When the user tries to connecteur a server konnector with
wrong identifiers and fix identifiers and retry conection,
the previous identifier a resent to the konnector and an
io.cozy.account document, not linked to any trigger is also
created for each atempt.
Now the trigger manager will reuse the account from the
the current ConnectionFlow instance when possible.