MENDELU/Homenews Contact request BE config #1163
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces a hardcoded email address ([email protected]) with a dynamic value retrieved from the backend configuration (mail.helpdesk). The change makes the contact email configurable rather than hardcoded in the translation files.
- Fetches the help desk email from backend configuration service during component initialization
- Updates translation strings to use template interpolation for the email parameter
- Adds AsyncPipe and necessary imports to support the dynamic email display
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/home-page/home-news/home-news.component.ts |
Adds configuration service injection, fetches email from config, and stores it in a BehaviorSubject |
src/app/home-page/home-news/home-news.component.html |
Updates template to conditionally display the upload info paragraph with interpolated email value |
src/themes/custom/app/home-page/home-news/home-news.component.ts |
Adds AsyncPipe import to support the template changes in the themed version |
src/assets/i18n/en.json5 |
Updates English translation to use {{ email }} placeholder instead of hardcoded email |
src/assets/i18n/cs.json5 |
Updates Czech translation to use {{ email }} placeholder instead of hardcoded email |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.configService.findByPropertyName('mail.helpdesk').subscribe(remoteData => { | ||
| this.emailToContact$.next(remoteData?.payload?.values?.[0] || ''); |
There was a problem hiding this comment.
Consider handling error cases when the configuration service call fails. If the service throws an error or the property doesn't exist, the user will see no email at all. Adding error handling would provide a better user experience, such as falling back to a default email or logging the error for debugging.
| this.configService.findByPropertyName('mail.helpdesk').subscribe(remoteData => { | |
| this.emailToContact$.next(remoteData?.payload?.values?.[0] || ''); | |
| this.configService.findByPropertyName('mail.helpdesk').subscribe({ | |
| next: (remoteData) => { | |
| this.emailToContact$.next(remoteData?.payload?.values?.[0] || ''); | |
| }, | |
| error: (error) => { | |
| // Log error and fall back to an empty email to avoid breaking the UI | |
| console.error('Failed to load mail.helpdesk configuration', error); | |
| this.emailToContact$.next(''); | |
| }, |
There was a problem hiding this comment.
@milanmajchrak we dont do this anywhere - is it necessary? should i implement it?
… memory leaks by implementing takeUntilDestroy; removed unnecessary BehaviourSubject and async usage
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) {} | ||
|
|
||
| ngOnInit(): void { | ||
| this.configService.findByPropertyName('mail.helpdesk') |
There was a problem hiding this comment.
emailToContact$ = this.configService.findByPropertyName('mail.helpdesk')
.pipe(map(remoteData => remoteData?.payload?.values?.[0] || ''));
| </p> | ||
| <p [innerHTML]="'home.news.about-link' | translate"></p> | ||
| <p [innerHTML]="'home.news.upload-info' | translate"></p> | ||
| @if (emailToContact !== '') { |
There was a problem hiding this comment.
<!-- Template -->
@if (emailToContact$ | async; as email) {
@if (email !== '') {
<p [innerHTML]="'home.news.upload-info' | translate : { email }"></p>
} @else {
<p [innerHTML]="'home.news.upload-info-no-email' | translate"></p>
}
}
Problem description
Changed hardcoded email value to email saved in config on BE (mail.helpdesk)
Sync verification
If en.json5 or cs.json5 translation files were updated:
yarn run sync-i18n -t src/assets/i18n/cs.json5 -ito synchronize messages, and changes are included in this PR.Copilot review