-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Changing current user loading method using async promises #3786
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
Conversation
@iamareebjamal Tested on local, with production environment too (Fastboot_Disabled=false) . |
Will test today |
@iamareebjamal Merge? |
@iamareebjamal Please have a look, Fixes the transition to previous route too! |
|
Both redirection and login is working well with fastboot enabled |
this.set('session.previousRouteName', null); | ||
} | ||
|
||
return this._loadCurrentUser(); |
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.
Why call this here?
On event CFS page, went to login page, logged in and got |
This is still happening as reported above |
I will check if there are other places where user is called before loading
and use the service to resolve them
…On Wed, 15 Jan, 2020, 23:25 Areeb Jamal, ***@***.***> wrote:
Also, there was a bug that sometimes the transition happens before user is
loaded, so the site crashes. Test for that in fastboot mode as well
This is still happening as reported above
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3786?email_source=notifications&email_token=AKQMTLTAOXFV3VGWOUWVEI3Q55EYPA5CNFSM4KFNFOG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJBGSZY#issuecomment-574777703>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLV2PHR22UNB665CYFTQ55EYPANCNFSM4KFNFOGQ>
.
|
That's not the solution. Solution should be generic. User should be loaded before transitioning |
@iamareebjamal Please try it again, fixes the transition crashes. |
app/controllers/public/index.js
Outdated
} | ||
} finally { | ||
this.set('isLoading', false); | ||
} |
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 not the solution
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.
What should be the approach then?
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 doesn't even fix the issue I mentioned. So the approach should be whatever which doesn't at least break the existing app
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.
Wrapping await calls inside try-catch which, if fail were crashing fastboot app server ?
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.
No, that did not fix the issue, this is called ignoring the issue, not fixing it. Besides, it is still crashing
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.
Nobody asked you to go to eventyay. Change API_HOST to https://api.eventyay.com and retest
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.
@iamareebjamal Working on fastboot, Also checked on fossasia summit 2020
. There was an issue in loading currentUser which was resulting into null query. Took reference from your WIP PR. It is working well now
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 should be reverted
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.
The try catch block ?
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.
@iamareebjamal done!
Complexity increasing per file
==============================
- app/services/current-user.js 2
See the complete overview on Codacy |
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.
Great work
Fixes #3677
Fixes #2396