Skip to content

Feature/AM-51 - #135

Open
ErickNaunay wants to merge 12 commits into
masterfrom
feature/AM-51
Open

ErickNaunay wants to merge 12 commits into
masterfrom
feature/AM-51

Conversation

@ErickNaunay

Copy link
Copy Markdown
Contributor

Description

Expose user manager through the REST API

Stories

https://bixlabs.atlassian.net/browse/AM-51?atlOrigin=eyJpIjoiNTA3ZmRkMWUyMzZhNGM3Zjg1YWYxNDQ1NTMyZGNjMTAiLCJwIjoiaiJ9

List of changes

  • Add user manager application logic
  • Separate routes from authentication and management

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

git pull --prune
git checkout <feature/AM-51>
make run

Impacted Areas in Application

  • Application layer
  • Authentication REST API routes
  • User manager

Migrations

NO

@ErickNaunay
ErickNaunay requested a review from jac1013 November 3, 2020 16:51
@sonarqubecloud

sonarqubecloud Bot commented Nov 3, 2020

Copy link
Copy Markdown

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
19.4% 19.4% Duplication

@jac1013 jac1013 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good 👍 , minor comments to discuss/address.

func configureUserManagerRoutes(restConfig userManagerRESTConfigurator, r *gin.Engine) *gin.Engine {
router := r.Group("/v1/users")

router.POST("/search", restConfig.findOne)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/search sounds like a big name for this, shouldn't be something like /find_by_email? (I don't know the convention that we are using for naming endpoints, consider that).

Another thing, I don't think this should be a POST, but a GET, are you doing this to prevent showing the email in the URL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! I think the same /search is abig name for a simple endpoint

Yes, First tried to use GET but swagger denies all GET request with a body, they don't allow those calls. But also the email is sensitive information, therefore it shouldn't be visible on the URL

Comment thread api/usermanager/user_manager.go
@jac1013

jac1013 commented Feb 12, 2021

Copy link
Copy Markdown
Member

I need to fix whatever is pending here so I'll have to do this next week.

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.

2 participants