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

Relationship with Authority - tests... #22105

Closed

Conversation

OmarHawk
Copy link
Contributor

@OmarHawk OmarHawk commented May 12, 2023

whether there's a problem to have a ManyToMany relationship with Authority (similar as User) using the new with builtInEntity

#22106

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

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

jdl/jdl-importer.spec.ts Outdated Show resolved Hide resolved
@mshima
Copy link
Member

mshima commented May 12, 2023

Failure is related to snapshots only.

@OmarHawk
Copy link
Contributor Author

Failure is related to snapshots only.

Mh... true, one of the Snapshots was incorrectly set up. Still it won't work unfortunately. I'd try to look into it next week unless you are faster with that and want to do that by yourself ;)

@OmarHawk OmarHawk force-pushed the bugfix/relationship-with-authority branch from 132d709 to 82c5cb0 Compare May 31, 2023 08:06
@OmarHawk OmarHawk force-pushed the bugfix/relationship-with-authority branch from 82c5cb0 to f53d9da Compare July 20, 2023 14:33
@DanielFran DanielFran closed this Oct 31, 2023
@DanielFran DanielFran reopened this Oct 31, 2023
@DanielFran
Copy link
Member

@OmarHawk Can you please confirm if everything has been fixed?

@OmarHawk
Copy link
Contributor Author

OmarHawk commented Oct 31, 2023

I'll rebase on Thursday and let the tests (which is the only thing until now in this pull request) run again.

@mshima
Copy link
Member

mshima commented Oct 31, 2023

It's needed to add this for Authority:

export function createUserEntity(customUserData = {}, application) {

@OmarHawk OmarHawk force-pushed the bugfix/relationship-with-authority branch from f53d9da to 4ae9c02 Compare November 7, 2023 11:03
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Nov 7, 2023

It's needed to add this for Authority:

export function createUserEntity(customUserData = {}, application) {

This is then really only about having it declared and then putting it like that?

loadUser({ application, entitiesToLoad }) {
if (application.generateBuiltInUserEntity) {
if (this.sharedData.hasEntity('User')) {
throw new Error("Fail to bootstrap 'User', already exists.");
}
const customUser = entitiesToLoad.find(entityToLoad => entityToLoad.entityName === 'User');
const customUserData = customUser ? customUser.entityStorage.getAll() : {};
const user = createUserEntity.call(this, customUserData, application);
this.sharedData.setEntity('User', user);
(application as any).user = user;
}
},

The rest of the User entity stuff is also happening somewhere else... I wonder, why the User entity was not "simply" ;-) declared as "actual" jdl String and merged with the user defined jdl (if existing), so that it would be treated just like any other entity and could then be used as such without special logic or requirement to use "with builtInEntity" - and without having special / kind of duplicated logic/templates in a few places...

@mshima
Copy link
Member

mshima commented Nov 8, 2023

This is then really only about having it declared and then putting it like that?

Yes, should be.
May need specific customizations to add management.

The rest of the User entity stuff is also happening somewhere else... I wonder, why the User entity was not "simply" ;-) declared as "actual" jdl String and merged with the user defined jdl (if existing), so that it would be treated just like any other entity and could then be used as such without special logic or requirement to use "with builtInEntity" - and without having special / kind of duplicated logic/templates in a few places...

JDL does very few checks now. Consistency checks is one of them. You cannot have a relationship without both sides Entities.
with builtInEntity ignores that check.
v8 focused a lot in modularization and blueprints.
At v7 validation was determined by application configs.
This causes problems like for e.g with oauth2 auth, builtIn User exists in JHipster. None of backend blueprints implements it.

Declaring User entity instead of with builtInEntity should work and is merged to built-in definition.

entity User {}
entity Foo {}
relationship ManyToOne {
  Foo to User
}

User template is separate. So any additional field or ownerSide relationship are silently ignored. Generating User using common templates and making it customizable is doable.

About all existing logic, we have a lot of legacy code that are slowly been cleaned up.

@OmarHawk OmarHawk force-pushed the bugfix/relationship-with-authority branch from 4ae9c02 to 20bcc66 Compare December 22, 2023 09:12
@OmarHawk OmarHawk force-pushed the bugfix/relationship-with-authority branch from 20bcc66 to 70b92f3 Compare January 23, 2024 16:03
@mshima mshima mentioned this pull request Jan 25, 2024
6 tasks
@deepu105 deepu105 added this to the 8.2.0 milestone Mar 20, 2024
@OmarHawk OmarHawk deleted the bugfix/relationship-with-authority branch March 25, 2024 06:15
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