chore: Migrate all the hardcoded messages and emails in english to bundle.properties for password reset service.#12544
Conversation
| notification.email.passwordResetCompleted.subject=Dataverse Password Reset Successfully Changed | ||
| notification.email.datasetWasCreated=Dataset "<a href = "{0}">{1}</a>" was just created by {2} in the {3} collection. | ||
| notification.email.datasetWasMoved=Dataset "<a href = "{0}">{1}</a>" was just moved by {2} to the {3} collection. | ||
| notification.email.requestedFileAccess=You have requested access to a file(s) in dataset "<a href = "{0}">{1}</a>". Your request has been sent to the managers of this dataset who will grant or reject your request. If you have any questions, you may reach the dataset managers using the "Contact" link on the upper right corner of the dataset page. |
There was a problem hiding this comment.
I'm just putting this at the bottom.
At https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-12544/1/consoleFull there was an unrelated failure:
TASK [dataverse : install docker-ce repo] **************************************
fatal: [localhost]: FAILED! => {"changed": false, "dest": "/etc/yum.repos.d/docker-ce.repo", "elapsed": 0, "gid": 0, "group": "root", "mode": "0644", "msg": "Connection failure: [ASN1: NOT_ENOUGH_DATA] not enough data (_ssl.c:4192)", "owner": "root", "secontext": "system_u:object_r:system_conf_t:s0", "size": 811, "state": "file", "uid": 0, "url": "https://download.docker.com/linux/centos/docker-ce.repo"}
I kicked off https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-12544/2/
There was a problem hiding this comment.
Hello @pdurbin Thanks for looking into the PR. You want the new properties added to the bottom of the file Bundle.properties?
There was a problem hiding this comment.
@mahendrapaipuri thanks for the PR! No, where you put them is fine. Thanks!
There was a problem hiding this comment.
This week we actually turned off the Jenkins job. We switched to GitHub Actions. So I'll merge or rebase the branch, which should remove any report from Jenkins from this PR.
There was a problem hiding this comment.
Done. Running. Oh, I also just noticed that because @mahendrapaipuri is a new contributor, GitHub Actions hadn't run. I just clicked the button the approve them. I'll move this PR back to "In Review" until those test pass.
|
We talked about this yesterday during Triage Tuesday. It's a no-brainer that we want this! |
rtreacy
left a comment
There was a problem hiding this comment.
Claude
PR #12544 Review: Migrate password reset messages to Bundle.properties
What it does: Replaces 6 hardcoded English strings (UI messages + a notification email body/subject) in PasswordResetServiceBean.attemptPasswordReset() with BundleUtil.getStringFromBundle() lookups backed by new keys in Bundle.properties. Small, self-contained change (+18/-10, 3 files) with a release note. Closes #12536.
Correctness
- All 6 replaced strings map correctly to new bundle keys, and the property values are copied verbatim from the original hardcoded text — no wording drift.
- The new email pattern notification.email.passwordResetCompleted uses {0} for authUser.getName() and is formatted via MessageFormat.format(pattern, paramArray) — this exactly mirrors the existing sendPasswordResetEmail() method a few dozen lines above (notification.email.passwordReset / .subject), so it's consistent with the file's established pattern rather than a new idiom.
- No apostrophes/quotes in the new property values, so no MessageFormat escaping ('') issues — good, since that's a common gotcha in this file (see passwdReset.noEmail.tip a few lines above, which does need it).
- BundleUtil.getStringFromBundle(key) doesn't require a FacesContext/container, so this remains safe to call from a service bean and in the existing plain unit tests (PasswordResetServiceBeanTest) — I checked and none of those tests assert on the literal message text, so nothing breaks.
Style / conventions
- Follows the existing passwdReset.* / notification.email.* key naming conventions used elsewhere in Bundle.properties.
- Minor nit: String[] paramArray = {authUser.getName() }; has a stray space before the closing brace — purely cosmetic, and matches the (also slightly inconsistent) spacing already present in the neighboring sendPasswordResetEmail method, so not a regression.
- Only the English Bundle.properties was touched, which is correct per Dataverse convention — translated bundles are updated via Transifex separately, not in the same PR.
Test coverage
- No new tests were added, but none were needed — this is a mechanical string-externalization change with identical behavior, and existing tests in PasswordResetServiceBeanTest already exercise all the branches touched (null user, null password, save failure, success path) without depending on exact string values.
Risks
- None significant. This is a low-risk, well-scoped i18n cleanup. The only latent (pre-existing, out of scope) oddity is at line 231-233 where messageSummary/third constructor arg ordering looks slightly off (parseMessages(errors) result assigned to messageSummary, then messageSummaryFail passed as the detail argument) — this bug predates this PR and isn't touched by it, just worth flagging separately if not already known.
Verdict: Clean, low-risk i18n migration that matches existing patterns in the file. Fine to merge as-is; the spacing nit isn't worth blocking on.
…ndle.properties for password reset service. Closes IQSS#12536 Signed-off-by: Mahendra Paipuri <mahendra.paipuri@gmail.com>
9487fbd to
d0e6531
Compare
What this PR does / why we need it: Improve internationalization
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: