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

[Angular] Add account with authentication JWT #1091

Merged
merged 4 commits into from
Apr 8, 2022
Merged

[Angular] Add account with authentication JWT #1091

merged 4 commits into from
Apr 8, 2022

Conversation

qmonmert
Copy link
Contributor

Fix #898

@qmonmert qmonmert marked this pull request as ready for review March 19, 2022 14:18
@codecov
Copy link

codecov bot commented Mar 19, 2022 •
edited
Loading

Codecov Report

Merging #1091 (8ed15d9) into main (02bb7c8) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##                main     #1091   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      1503      1515   +12     
===========================================
  Files            283       283           
  Lines           4829      4901   +72     
  Branches          88        88           
===========================================
+ Hits            4829      4901   +72
Impacted Files Coverage Δ
...ore/infrastructure/primary/rest/ReactResource.java 100.00% <ø> (ø)
...s/infrastructure/primary/CypressReactResource.java 100.00% <ø> (ø)
...re/infrastructure/primary/rest/SvelteResource.java 100.00% <ø> (ø)
.../core/infrastructure/primary/rest/VueResource.java 100.00% <ø> (ø)
...ar/core/application/AngularApplicationService.java 100.00% <100.00%> (ø)
.../generator/client/angular/core/domain/Angular.java 100.00% <100.00%> (ø)
...ient/angular/core/domain/AngularDomainService.java 100.00% <100.00%> (ø)
...e/infrastructure/primary/rest/AngularResource.java 100.00% <100.00%> (ø)
...p/springboot/secondary/client/AngularRepository.ts 100.00% <100.00%> (ø)
...app/springboot/secondary/client/ReactRepository.ts 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32ece27...8ed15d9. Read the comment docs.

@qmonmert qmonmert force-pushed the feature/898 branch 2 times, most recently from 9102064 to b809574 Compare March 19, 2022 20:03
@qmonmert qmonmert requested a review from pascalgrimaud March 19, 2022 21:24
@qmonmert qmonmert force-pushed the feature/898 branch 2 times, most recently from df2ac0b to 44c88e3 Compare March 20, 2022 19:18
@pascalgrimaud
Copy link
Member

@qmonmert : you need to add the new API in generator.sh, otherwise, it won't be tested in CI

@pascalgrimaud pascalgrimaud marked this pull request as draft March 21, 2022 07:06
@qmonmert
Copy link
Contributor Author

@pascalgrimaud where is this file? I don't see yet I rebase main branch

@pascalgrimaud
Copy link
Member

@qmonmert : here is the file https://github.com/jhipster/jhipster-lite/blob/main/tests-ci/generate.sh#L183

You need to call your new API here

@qmonmert
Copy link
Contributor Author

@pascalgrimaud okay it's generate.sh not generator.sh :p

@pascalgrimaud
Copy link
Member

My bad, I'm sorry for this typo :-p

@qmonmert qmonmert force-pushed the feature/898 branch 4 times, most recently from 91cae49 to 8f845d4 Compare March 25, 2022 11:17
@qmonmert qmonmert closed this Mar 25, 2022
@qmonmert qmonmert reopened this Mar 25, 2022
@qmonmert qmonmert force-pushed the feature/898 branch 3 times, most recently from d64c9d9 to fed1cb7 Compare March 25, 2022 16:41
@qmonmert qmonmert closed this Mar 25, 2022
@qmonmert qmonmert reopened this Mar 25, 2022
@qmonmert qmonmert marked this pull request as ready for review March 25, 2022 17:10
@pascalgrimaud
Copy link
Member

I started to review it.
Globally, the authent works, but I think we need to reorganize all these files, as it does not totally respect Hexagonal Architecture.
I'll propose you how we can reorganize it, but it would be easier to propose it directly in a sample project, rather than the generated part.

@qmonmert
Copy link
Contributor Author

@pascalgrimaud ok

@pascalgrimaud
Copy link
Member

Here the repo I use for testing and reviewing:
pascalgrimaud/tmp-jhlite-angularapp#1

@pascalgrimaud
Copy link
Member

After studying the architecture, I don't have enough skill to propose a good folder to totally respect Hexagonal Architecture with Angular. So I think we can have features by package, a little bit similar to what we have in generator-jhipster.

Can you refactorize and use this plz?
image

In another PR, we could refactorize "common" to use another like "app", as it's not necessary to have 'primary' if we can't split correctly in primary/domain/secondary

Another comment:

  • I use angular styled, followed by angular+jwt and I lost all styled in app.component.html

@qmonmert
Copy link
Contributor Author

qmonmert commented Apr 3, 2022

@pascalgrimaud ok I will do that!

@qmonmert qmonmert force-pushed the feature/898 branch 6 times, most recently from 13dc2ca to c6684cf Compare April 4, 2022 18:51
@qmonmert
Copy link
Contributor Author

qmonmert commented Apr 4, 2022

@pascalgrimaud refacto done + fix angular styled jwt

pascalgrimaud
pascalgrimaud previously approved these changes Apr 8, 2022
View reviewed changes
src/main/java/tech/jhipster/lite/generator/client/angular/core/domain/AngularDomainService.java
Comment on lines +21 to +24
private static final String APP_MODULE = "app.module.ts";
private static final String APP_COMPONENT = "app.component.ts";
private static final String APP_COMPONENT_SPEC = "app.component.spec.ts";
private static final String APP_COMPONENT_HTML = "app.component.html";
Copy link
Member

Choose a reason for hiding this comment

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

can be moved to Angular.java

src/main/resources/generator/client/angular/proxy.conf.json
@@ -0,0 +1,6 @@
{
"/api": {
"target": "http://localhost:8080",
Copy link
Member

Choose a reason for hiding this comment

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

mhh I think it should be replaced by serverPort property

src/test/java/tech/jhipster/lite/generator/client/angular/core/application/AngularAssert.java
Comment on lines +75 to +83
assertFileExist(project, getPath(pathAuth, "account.model.ts"));
assertFileExist(project, getPath(pathAuth, "account.service.ts"));
assertFileExist(project, getPath(pathAuth, "account.service.spec.ts"));
assertFileExist(project, getPath(pathAuth, "auth-jwt.service.ts"));
assertFileExist(project, getPath(pathAuth, "auth-jwt.service.spec.ts"));
assertFileExist(project, getPath(pathAuth, "auth.interceptor.ts"));
assertFileExist(project, getPath(pathLogin, "login.service.ts"));
assertFileExist(project, getPath(pathLogin, "login.service.spec.ts"));
assertFileExist(project, getPath(pathLogin, "login.model.ts"));
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we can do better, by using Angular classes, with forEach

@pascalgrimaud pascalgrimaud enabled auto-merge April 8, 2022 12:35
@pascalgrimaud pascalgrimaud merged commit 42d8a8d into jhipster:main Apr 8, 2022
@qmonmert qmonmert mentioned this pull request Apr 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalgrimaud pascalgrimaud pascalgrimaud left review comments

Assignees
No one assigned
Labels
area: feature request 💡 client: angular
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Angular: account with authentication JWT
2 participants
@qmonmert @pascalgrimaud