Skip to content

Fix: save newsletter template#5285

Merged
sreichel merged 17 commits into
OpenMage:mainfrom
sreichel:newsletter
Apr 10, 2026
Merged

Fix: save newsletter template#5285
sreichel merged 17 commits into
OpenMage:mainfrom
sreichel:newsletter

Conversation

@sreichel
Copy link
Copy Markdown
Contributor

@sreichel sreichel commented Mar 1, 2026

Description (*)

Cant save existing newsletter template b/c "template_type" has to be an integer value, string given.

  • added getter/setter
  • added save success message
  • added cypress test

Related Pull Requests

Manual testing scenarios (*)

  1. go to admnin - newsletter - template
  2. save an existing template

Copilot AI review requested due to automatic review settings March 1, 2026 15:12
@github-actions github-actions Bot added Component: Core Relates to Mage_Core Component: Adminhtml Relates to Mage_Adminhtml Component: Newsletter Relates to Mage_Newsletter translations Relates to app/locale cypress labels Mar 1, 2026
@sreichel sreichel added the bug label Mar 1, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes saving existing newsletter templates by ensuring template_type is handled as an integer (matching validation expectations), and adds UI/test coverage for a successful save in the admin newsletter template editor.

Changes:

  • Add explicit getTemplateType()/setTemplateType() implementations in Mage_Newsletter_Model_Template to coerce template_type to int.
  • Add an admin session success message after saving a newsletter template.
  • Update Cypress e2e to cover saving an existing newsletter template and assert the success message.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cypress/e2e/openmage/backend/newsletter/template.cy.js Updates edit-route test to save an existing template and check for a success message.
app/locale/en_US/Mage_Newsletter.csv Adds a new translation string for the save success message.
app/code/core/Mage/Newsletter/Model/Template.php Introduces typed getter/setter for template_type to satisfy int validation.
app/code/core/Mage/Core/Model/Email/Template/Abstract.php Docblock updates for template text/type method annotations.
app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php Adds a save success message and returns immediately after redirect.

Comment thread cypress/e2e/openmage/backend/newsletter/template.cy.js Outdated
Comment thread app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php Outdated
Comment thread app/locale/en_US/Mage_Newsletter.csv
@github-actions github-actions Bot removed the Component: Core Relates to Mage_Core label Mar 11, 2026
@sreichel sreichel added this to the 20.17.0 milestone Apr 8, 2026
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 9, 2026

Comment thread app/code/core/Mage/Newsletter/Model/Template.php Outdated
Comment thread app/code/core/Mage/Newsletter/Model/Template.php
sreichel and others added 2 commits April 10, 2026 02:35
Co-authored-by: B3Hana <180432612+B3Hana@users.noreply.github.com>
…ntroller.php

Co-authored-by: B3Hana <180432612+B3Hana@users.noreply.github.com>
Comment thread cypress/e2e/openmage/backend/newsletter/template.cy.js Outdated
…ntroller.php

Co-authored-by: B3Hana <180432612+B3Hana@users.noreply.github.com>
Comment thread app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php Outdated
@sreichel sreichel requested a review from B3Hana April 10, 2026 01:33
@sreichel
Copy link
Copy Markdown
Contributor Author

@all-contributors add @B3Hana code

@allcontributors
Copy link
Copy Markdown
Contributor

@sreichel

I've put up a pull request to add @B3Hana! 🎉

@Hanmac
Copy link
Copy Markdown
Contributor

Hanmac commented Apr 10, 2026

validateType might not be needed, because validateChoice already restricts it to [1, 2]

But better safe than sorry?

@sreichel
Copy link
Copy Markdown
Contributor Author

validateType might not be needed, because validateChoice already restricts it to [1, 2]

But better safe than sorry?

Good one ... not sure/untested if validateChoice is type-safe. (1 vs "1").

@sreichel
Copy link
Copy Markdown
Contributor Author

Want to add a new test case?

@sreichel sreichel merged commit 20c4649 into OpenMage:main Apr 10, 2026
21 checks passed
@sreichel sreichel deleted the newsletter branch April 10, 2026 11:16
@Hanmac
Copy link
Copy Markdown
Contributor

Hanmac commented Apr 10, 2026

validateType might not be needed, because validateChoice already restricts it to [1, 2]
But better safe than sorry?

Good one ... not sure/untested if validateChoice is type-safe. (1 vs "1").

https://github.com/symfony/symfony/blob/5bcd98749de899a7de2e0052e9234ca028cde867/src/Symfony/Component/Validator/Constraints/ChoiceValidator.php#L100
checks:

} elseif ($constraint->match xor \in_array($value, $choices, true)) {

this says that strict makes it type-safe:
https://www.php.net/manual/en/function.in-array.php

If the third parameter strict is set to true then the in_array() function will also check the types of the needle in the haystack.

so no extra test case needed

@sreichel
Copy link
Copy Markdown
Contributor Author

PR to remove additional check?

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

Labels

bug Component: Adminhtml Relates to Mage_Adminhtml Component: Newsletter Relates to Mage_Newsletter cypress phpunit translations Relates to app/locale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants