Skip to content

Authorization refactoring#150

Closed
smacker wants to merge 4 commits into
src-d:masterfrom
smacker:refactoring_auth_fe_call
Closed

Authorization refactoring#150
smacker wants to merge 4 commits into
src-d:masterfrom
smacker:refactoring_auth_fe_call

Conversation

@smacker

@smacker smacker commented Feb 21, 2018

Copy link
Copy Markdown
Contributor

Fixes #99

Thanks @dpordomingo for good example which I could test and implement it a little bit "righter" way:

  • Keep frontend logic for auth out of components
  • Make UI more similar to how it works right now (all errors go to popup) and user is able to retry login again.

@smacker smacker requested a review from bzz February 21, 2018 14:25
@bzz bzz requested review from carlosms and dpordomingo February 21, 2018 16:13
@smacker smacker force-pushed the refactoring_auth_fe_call branch 2 times, most recently from c080736 to 7bef196 Compare February 28, 2018 12:44

@carlosms carlosms left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

My only concern is that we need to coordinate with the infra. team to change the current GitHub application callback, and this might be easy to forget.

Maybe we should open a new issue for the 0.0.3 release with a comment to remind us of this (ping @dpordomingo)

@smacker

smacker commented Feb 28, 2018

Copy link
Copy Markdown
Contributor Author

Good point about callback. Only it should be coordinated with admin of the application. (I'm not sure it's infra)

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@smacker smacker force-pushed the refactoring_auth_fe_call branch from 4829050 to 03c5af5 Compare February 28, 2018 17:37
@dpordomingo

Copy link
Copy Markdown
Contributor

superseded by original PR #142

@dpordomingo dpordomingo closed this Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants