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.

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

Fix React Travis build #7506

Merged
merged 10 commits into from
Apr 30, 2018
Merged

Fix React Travis build #7506

merged 10 commits into from
Apr 30, 2018

Conversation

chrisdns
Copy link
Member

@chrisdns chrisdns commented Apr 19, 2018

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@chrisdns chrisdns mentioned this pull request Apr 19, 2018
77 tasks
@mraible
Copy link
Contributor

mraible commented Apr 19, 2018

@chrisdns Please let me know if you have any questions about how OAuth should work. I coded a lot of it, so happy to help!

condition: generator => generator.authenticationType === 'oauth2',
path: REACT_DIR,
templates: [
'shared/reducers/user-service.ts'
Copy link
Member

Choose a reason for hiding this comment

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

please name it as user-management as service is used only in context of angular

@@ -89,7 +89,11 @@ Object.keys(differentRelationships).forEach(key => {
if (uniqueRel.otherEntityAngularName === 'User') {
_%>
import { I<%= uniqueRel.otherEntityAngularName %> } from 'app/shared/model/user.model';
<%_ if (authenticationType === 'oauth2') { _%>
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a stray file user-service.spec.ts.ejs in this PR. Can you check. Other than that, this PR is ready to merge

@@ -82,7 +82,7 @@ _%>
<%_ } _%>
<%_ if (databaseType === 'sql') { _%>

default <%= entityClass %> fromId(<% if (hasOAuthUser) { _%>String<%_ } else { _%>Long<% } %> id) {
default <%= entityClass %> fromId(Long id) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mraible is this change good ? It fixes the error incompatible types: java.lang.String cannot be converted to java.lang.Long with some entities and auth with oauth2 seems to works well but i may have missed something

Copy link
Contributor

Choose a reason for hiding this comment

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

If you choose OAuth/OIDC as your authenticationType, the user's id is a string. Is this breaking the build?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. When we create entity with mapper that has relationship with 'user' (in a project using oauth2 authentication), the related mapper class has a string as fromId() function argument and this is indeed breaking the build.
I tried to switch it to Long and it doesnt break anymore and authentication is still working.
It may be because some type changes are missing in other entity mapper related files

Copy link
Contributor

Choose a reason for hiding this comment

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

If it doesn't break anymore, I'm Ok with this change. FWIW, it looks like you need to resolve conflicts before this can be merged.

@jdubois
Copy link
Member

jdubois commented Apr 30, 2018

As this is approved by @deepu105 and @mraible let's do this

@jdubois jdubois merged commit 5d7e5e0 into jhipster:master Apr 30, 2018
@jdubois jdubois added this to the 5.0.0-beta.1 milestone May 3, 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.

4 participants