-
Notifications
You must be signed in to change notification settings - Fork 26
Fix exclude account login #494
Fix exclude account login #494
Conversation
src/_store/root.js
Outdated
actions.updateAppState('authorized', false); | ||
await actions.updateAppState('authorized', false); | ||
const errorMsg = e.message; | ||
if (errorMsg.indexOf('you have excluded yourself until') > -1) { |
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 probably wrong, will not work on translated message, or whenever backends change the msg, use error code?
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.
I prefer to handle this in tryAuth itself, as a recursive call, what do you think?
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.
Likely need to catch by error type, since you will not have this string when translated :)
const response = await api.authorize(token); | ||
let response = await api.authorize(token); | ||
if (response.error && response.error.code === 'SelfExclusion') { | ||
const account = window.BinaryBoot.accounts.filter(x => x.account.startsWith('VRTC')); |
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.
I think find
is more appropriate instead of filter
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.
Also, there is no guarantee that there will be a VRTC 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.
@borisyankov i think there would always be Virtual account wouldn't there ?
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.
Nope.
@nuruddeensalihu this have been here for 11 days, last activity is 6 days ago What's stopping it from being merge? |
@qingweibinary I don't see any issue with this PR, though boris said there are instance where VRTC account would not exist . If indeed there is, then there would be issue then. I mean could a user have only MF account without VRTC ? IF so then this solution would not work for such case. Otherwise this PR is ok. I have tested many times and no problem with it. |
@nuruddeensalihu I think it's pretty clear that boris said that VRTC does not always exists.
So I guess you will make some changes so that it handle cases where VRTC is absent, right ? |
@qingweibinary i am out of idea when he said VRTC does not exist in some instance. Thats something i have never thought of. So i was thinking if indeed it happened randomly that some account does not have VRTC then this PR would be obsolete. Btw what kind of account do legacy account use in place of VRTC ? I mean do they have an alternative or just no VRTC at all? |
@nuruddeensalihu if I remember correctly, there is no other account. |
Hi,
Attached few changes that should allow the Virtual account to be used incase of exclusion as mentioned here https://trello.com/c/hk1QgRpg/44-self-exclusion-behaviour .