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

Remove LocaleConfiguration #23818

Merged

Conversation

shrralis
Copy link
Contributor

@shrralis shrralis commented Oct 11, 2023

remove LocaleConfiguration due to being a leftover from angularjs

Fix #20106


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.

@shrralis
Copy link
Contributor Author

Not sure why it's failing

@DanielFran
Copy link
Member

@shrralis we are using snapshot to validate changes on file generation
You need to update snapshots and it should fix the issue
Also you need to add the deleted files to the cleanup list, so with the upgrade the files will be deleted too.

@mshima
Copy link
Member

mshima commented Oct 11, 2023

I think we should replace we our own header.
Spring i18n integration seems to be broken now.

@shrralis
Copy link
Contributor Author

shrralis commented Oct 11, 2023

@DanielFran sorry, it is my first time doing such kind of stuff.

You need to update snapshots and it should fix the issue

Do I understand correctly I need to run npm run update-snapshots and it would apply some changes to the source files I need to commit as well?

Also you need to add the deleted files to the cleanup list

It's about this function, right?

export default function cleanupOldServerFilesTask(this: BaseGenerator, taskParam: GeneratorDefinition['writingTaskParam']) {

If yes, then which version should I set there? Should it be under the currently latest defined 7.10.0?

@mshima I don't get what's wrong with Spring i18n. I believe it's being used by sending E-Mails only? If so, then there should be no problem since we creating a new Context(Locale.forLanguageTag(user.getLangKey())) and it's not dependent on the LocaleConfiguration we're removing (and I've re-checked, Spring i18n works as expected in E-Mails with my changes)

@mshima
Copy link
Member

mshima commented Oct 11, 2023

Translated error messages in api responses.
Would be nice to check if it resolves Accept-Language headers at least.
@DanielFran what do you think?

@shrralis
Copy link
Contributor Author

shrralis commented Oct 11, 2023

@mshima well, I could revert the change and remove only this part:

@Bean
public LocaleResolver localeResolver() {
    return new AngularCookieLocaleResolver("NG_TRANSLATE_LANG_KEY");
}

But still, I see that there's a LocaleChangeInterceptor (LocaleChangeFilter) configured which seems to be completely redundant to me because I couldn't find any usage of the language request param in the front-end part, so the locale never gets changed from what I see.

Would be nice to check if it resolves Accept-Language headers at least.

There's an AcceptHeaderLocaleResolver for that and it is being used in Spring by default:

https://github.com/spring-projects/spring-framework/blob/bb23032a789d2110a0515a8ea62550fd01b75b71/spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/WebMvcConfigurationSupport.java#L1156-L1159

@shrralis shrralis force-pushed the fix-20106-drop-AngularCookieLocaleResolver branch 2 times, most recently from e3006ee to ea5dc4a Compare October 11, 2023 19:10
@shrralis shrralis marked this pull request as ready for review October 11, 2023 20:30
@DanielFran
Copy link
Member

DanielFran commented Oct 12, 2023

Translated error messages in api responses. Would be nice to check if it resolves Accept-Language headers at least. @DanielFran what do you think?

@mshima Not sure it is working today?
@shrralis Can you check with/without your changes if it works?

@shrralis
Copy link
Contributor Author

@DanielFran I'm not sure what I should check there. From what I saw, API error messages aren't i18n-ized and all of the i18n is being performed by frontend frameworks (except EMail sending).

@mraible
Copy link
Contributor

mraible commented Apr 29, 2024

@shrralis Can you please resolve conflicts in this PR?

@shrralis shrralis closed this Apr 29, 2024
@shrralis shrralis force-pushed the fix-20106-drop-AngularCookieLocaleResolver branch from ea5dc4a to 31dae68 Compare April 29, 2024 20:06
@shrralis shrralis reopened this Apr 29, 2024
@shrralis
Copy link
Contributor Author

@mraible sure, done

remove LocaleConfiguration due to be a leftover from angularjs

Fix jhipster#20106
@shrralis shrralis force-pushed the fix-20106-drop-AngularCookieLocaleResolver branch from 450489d to b837857 Compare April 29, 2024 21:14
@mraible mraible merged commit 7865a27 into jhipster:main Apr 30, 2024
52 checks passed
@mraible
Copy link
Contributor

mraible commented Apr 30, 2024

@shrralis Thanks for your contribution!

@mraible mraible added this to the 8.4.0 milestone Apr 30, 2024
@shrralis
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop AngularCookieLocaleResolver or rename
4 participants