Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pivotal-176168841: Allow to check inactive user registration #315

Merged
merged 6 commits into from
Jan 18, 2021
Merged

Pivotal-176168841: Allow to check inactive user registration #315

merged 6 commits into from
Jan 18, 2021

Conversation

Juan-EBI
Copy link
Contributor

jhoanmanuelms
jhoanmanuelms reviewed Dec 16, 2020
View reviewed changes
Copy link
Contributor

@jhoanmanuelms jhoanmanuelms left a comment

Choose a reason for hiding this comment

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

I left some comments.

submission/submission-security/src/main/kotlin/ebi/ac/uk/security/service/SecurityService.kt Outdated
@@ -77,6 +78,10 @@ class SecurityService(
}
}

override fun checkUserRegistration(register: CheckUserRequest): SecurityUser {
return getOrCreateInactive(register.userEmail, register.userEmail)
Copy link
Contributor

Choose a reason for hiding this comment

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

The second parameter should be the name

submission/submission-webapp/src/main/kotlin/ac/uk/ebi/biostd/security/web/SecurityResource.kt Outdated
@@ -45,6 +46,15 @@ class SecurityResource(
securityService.registerUser(register)
}

@PostMapping(value = ["/check-user"])
@ApiOperation("Check user registration")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if we're going with this approach, the documentation should be better something like:

Checks if a user with the given email is registered in the system. In case it's not, a new user will be created using such email and the given user name. The new user will be inactive and should be activated via activation link.

submission/submission-webapp/src/main/kotlin/ac/uk/ebi/biostd/security/web/SecurityResource.kt Outdated
@@ -45,6 +46,15 @@ class SecurityResource(
securityService.registerUser(register)
}

@PostMapping(value = ["/check-user"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name for this particular endpoint is very misleading. We should go for something like check-register or search-register; something that indicates that this endpoint actually performs two operations.

Said this, I'm not sure about how REST compliant this is, maybe we need just to have a check endpoint and let them decide whether or not to use the already existing register endpoint.

Besides, the actual registration endpoint has a lot of info that this one doesn't like the password, whether or not the notifications are enabled, etc. How will this be handled later for the user? I'm just worried this could become a workaround for our actual registration process and we should be careful now that our API is having external users.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahaha , the issue with the registration one is that is used for humans this is only for machine integration that need to register users. As array express or PMC so as we do not want to for example allow then to provide i.e password or other information that they do not have this is the endpoint for that :).

I agree that check-registeris a much better name

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

jhoanmanuelms
jhoanmanuelms approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@jhoanmanuelms jhoanmanuelms left a comment

Choose a reason for hiding this comment

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

LGTM. Please consider my comment.

submission/submission-webapp/src/main/kotlin/ac/uk/ebi/biostd/security/web/SecurityResource.kt Outdated
@@ -45,6 +46,17 @@ class SecurityResource(
securityService.registerUser(register)
}

@PostMapping(value = ["/check-registration"])
@ApiOperation("Checks if a user with the given email is registered in the system. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use """ in order to avoid concatenation

@Juan-EBI
Merge branch 'master' of github.com:EBIBioStudies/biostudies-backend-…
4bd9099 
…services into feature/pivotal-176168841-Allow-to-check-inactive-user-registration
@Juan-EBI
code review changes
08ce478
@Juan-EBI
Merge branch 'master' of github.com:EBIBioStudies/biostudies-backend-…
727b9a7 
…services into feature/pivotal-176168841-Allow-to-check-inactive-user-registration
@Juan-EBI Juan-EBI merged commit 6015336 into master Jan 18, 2021
@jhoanmanuelms jhoanmanuelms deleted the feature/pivotal-176168841-Allow-to-check-inactive-user-registration branch February 8, 2021 10:06
jhoanmanuelms added a commit that referenced this pull request Mar 20, 2021
@jhoanmanuelms @Juan-EBI
Promote BETA (#353)
96edbce 
Feature
* Pivotal ID # 176052254: JSON Submission Attributes Validation (#310)
* Pivotal #ID:175163839: Submission Query Operations In Mongo (#319)
* Pivotal-176168841: Allow to check inactive user registration (#315)
* Pivotal ID # 176522202: Ext Submission serialization (#322)
* Pivotal ID # 172671360: Warn users about upcoming public release (#324)
* Pivotal #ID 176479707: Mongo implement draft funtionality (#327)
* Pivotal #ID 176479720: Mongo Metadata Query (#328)
* Pivotal ID # 175163839: Mongo Submission Stats (#331)
* Pivotal ID # 176886357: Async Submission Request (#334)
* Pivotal ID # 176977556: Delete Submissions In Mongo (#341)
* Pivotal ID #176679139: Ext Submission File List Refactor (#344)
* Pivotal ID # 177042374: Delete Draft After Submission (#349)
* Pivotal ID# 174948695: PTSubmit Delete Operation (#351)
* Pivotal #ID 177076728: Use PMC new streaming API (#346)
* Pivotal ID # 177267343: Validate EU-ToxRisk Submissions (#352)

Bugfix
* Pivotal ID # 176052264: Inner Folder Mapping (#311)
* Pivotal ID # 172671360: Ignore Empty Value Attributes (#314)
* Pivotal ID # 176377632: File list extension is missing in BIA studies (#321)
* Pivotal #ID 174642598: TSV Double Quotes (#350)
* Pivotal ID # 176501169: FTP Folders Missing (#335)

Chore
* Pivotal ID # 176377544: Add refresh users directory structure api endpoint (#320)
* Pivotal #ID 176479473:  Migrate biostudies test to TestContainers (#323)
* Pivotal ID # 176779952: Mongo iTests CI (#332)
* Pivotal #Id 176838189: Rename PMC mongo collections (#333)
* Pivotal #ID: 177040448: Store request as plain json (#336)
* Pivotal ID #175894089: Check if queues in RabbitMQ are persistent (#339)
* Pivotal ID #177074116 : model table section properly (#338)
* Pivotal ID # 176886824: Improve API Docs (#337)
* Pivotal #ID 177193125: Improve collection template validation (#345)

Co-authored-by: Juan <jcamilorada@ebi.ac.uk>
jhoanmanuelms added a commit that referenced this pull request Mar 21, 2021
@jhoanmanuelms @Juan-EBI
Promote to PROD (#354)
9eb4158 
Feature
* Pivotal ID # 176052254: JSON Submission Attributes Validation (#310)
* Pivotal #ID:175163839: Submission Query Operations In Mongo (#319)
* Pivotal-176168841: Allow to check inactive user registration (#315)
* Pivotal ID # 176522202: Ext Submission serialization (#322)
* Pivotal ID # 172671360: Warn users about upcoming public release (#324)
* Pivotal #ID 176479707: Mongo implement draft funtionality (#327)
* Pivotal #ID 176479720: Mongo Metadata Query (#328)
* Pivotal ID # 175163839: Mongo Submission Stats (#331)
* Pivotal ID # 176886357: Async Submission Request (#334)
* Pivotal ID # 176977556: Delete Submissions In Mongo (#341)
* Pivotal ID #176679139: Ext Submission File List Refactor (#344)
* Pivotal ID # 177042374: Delete Draft After Submission (#349)
* Pivotal ID# 174948695: PTSubmit Delete Operation (#351)
* Pivotal #ID 177076728: Use PMC new streaming API (#346)
* Pivotal ID # 177267343: Validate EU-ToxRisk Submissions (#352)

Bugfix
* Pivotal ID # 176052264: Inner Folder Mapping (#311)
* Pivotal ID # 172671360: Ignore Empty Value Attributes (#314)
* Pivotal ID # 176377632: File list extension is missing in BIA studies (#321)
* Pivotal #ID 174642598: TSV Double Quotes (#350)
* Pivotal ID # 176501169: FTP Folders Missing (#335)

Chore
* Pivotal ID # 176377544: Add refresh users directory structure api endpoint (#320)
* Pivotal #ID 176479473:  Migrate biostudies test to TestContainers (#323)
* Pivotal ID # 176779952: Mongo iTests CI (#332)
* Pivotal #Id 176838189: Rename PMC mongo collections (#333)
* Pivotal #ID: 177040448: Store request as plain json (#336)
* Pivotal ID #175894089: Check if queues in RabbitMQ are persistent (#339)
* Pivotal ID #177074116 : model table section properly (#338)
* Pivotal ID # 176886824: Improve API Docs (#337)
* Pivotal #ID 177193125: Improve collection template validation (#345)

Co-authored-by: Juan <jcamilorada@ebi.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhoanmanuelms jhoanmanuelms jhoanmanuelms approved these changes

Assignees

@Juan-EBI Juan-EBI

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Juan-EBI @jhoanmanuelms