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

add firstName and lastName to UserDTO and IUser #14229

Merged
merged 7 commits into from
Mar 11, 2021
Merged

Conversation

VergilSkye
Copy link
Contributor

@VergilSkye VergilSkye commented Mar 8, 2021

As discussed in #14164. firstName and lastName were removed from the user for security privacy reasons, so they cannot be used as a display fields.

this pr returns the old behavior and allows firstName and lastName to be used, but I don't think it should be merged without discussing options.

in my view, we have 3 options

  1. Don't merge this pr and and classify this as custom behavior. Focus on security privacy

  2. Merge this pr and rollback the old behavior. Old jdls, which use these fields, can be imported with as little friction as possible. Focus on backward compatibility

  3. Allow jdl using firstName and lastName but dont change User and IUser. this will break the code but we can create a tip on how to make the code work again write custom code ( how to change User java, integration test and IUser class). Balance between 1 and 2.


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

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Additional fields should not be generated by default but on demand.

@VergilSkye
Copy link
Contributor Author

Thanks for the reply and I added all changes @mshima but the PublicUserResourceIT it's crashing because the DEFAULT_FIRSTNAME and DEFAUL_LASTNAME has wrong constants.
the following code:

 <%_ for (field of _.sortedUniqBy(user.otherRelationships.map(relationship => relationship.relatedField), field => field.fieldName)) { _%>
        private static final String DEFAULT_<%= field.fieldName.toUpperCase() %> = "<%= field.fieldName %>";
    <%_ } _%>

generates:
private static final String DEFAULT_FIRSTNAME = "firstName";
private static final String DEFAULT_LASTNAME = "lastName";

but should be:
private static final String DEFAULT_FIRSTNAME = "john";
private static final String DEFAULT_FIRSTNAME = "doe";

Should i adding some function to convertToConstants ?
where it can be added?

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

There is no test case that uses those custom fields, not sure is a feature we must add tests for.
I am supposing it's working.

@VergilSkye
Copy link
Contributor Author

yeah, i don't know the jhipster policy for tests, but i already know a edge case.

if a end user create project and trying to import jdl that uses firtsName as display field, the jdl will be generate but the code will be broken. The end user need to regenerate the entity User.
I use this command to regenerate User jhipster --force --with-entities

@mshima mshima closed this Mar 11, 2021
@mshima mshima reopened this Mar 11, 2021
@mshima
Copy link
Member

mshima commented Mar 11, 2021

With this PR it's the developer choice what fields to expose. It's not a privacy problem.

@mshima mshima merged commit ac70eb6 into jhipster:main Mar 11, 2021
@mshima
Copy link
Member

mshima commented Mar 11, 2021

Thanks @VergilSkye

@VergilSkye
Copy link
Contributor Author

Thanks @mshima for your help, guidance and all the work done on the Jhipster project.

I am very excited about V7 and for having a small participation in this release.

@pascalgrimaud pascalgrimaud added this to the v7.0.0 milestone Mar 13, 2021
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