Send the merchant notification by default on a fresh install - #104
Draft
boo-code wants to merge 1 commit into
Draft
Send the merchant notification by default on a fresh install#104boo-code wants to merge 1 commit into
boo-code wants to merge 1 commit into
Conversation
install() registered its hooks but seeded no configuration, so CONTACTFORM_SEND_NOTIFICATION_EMAIL and CONTACTFORM_SEND_CONFIRMATION_EMAIL did not exist on a new shop. Both are read with Configuration::get() before anything is sent, and the send block is guarded by their disjunction, so a freshly installed contact form accepted messages and mailed nobody - the merchant was never told a message had arrived. Seed the two settings, with different defaults. The notification goes to the shop's own contact address and is the reason the form exists, so it is on. The confirmation goes to whatever address the visitor typed, which is what makes the form usable to mail a third party, so it stays an explicit opt-in and is off. Uninstalling a module does not remove its configuration, so install() runs again over existing values on a reset or a reinstall. seedDefault() therefore writes only when no value exists yet, and tests that through Configuration::get(), which cascades shop -> shop group -> global the way the module's own reads do. Configuration::hasKey() inspects a single level and would report "absent" under multistore for a value set globally or on the shop group, overriding a choice the merchant had already made.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
install()registered the module's two hooks and seeded no configuration, soCONTACTFORM_SEND_NOTIFICATION_EMAILandCONTACTFORM_SEND_CONFIRMATION_EMAILdid not exist on a new shop. Both are read withConfiguration::get()and the send block is guarded by their disjunction, so a freshly installed contact form accepted a message and mailed nobody: the merchant was never told it had arrived, and nothing on the page said why. This seeds the two settings with different defaults. The notification goes to the shop's own contact address and is the reason the form exists, so it is on. The confirmation goes to whatever address the visitor typed, which is what makes the form usable to mail a third party, so it stays an explicit opt-in and is off. Uninstalling does not remove a module's configuration, soinstall()runs again over existing values on a reset or a reinstall;seedDefault()writes only when no value exists yet and tests that withConfiguration::get(), which cascades shop -> shop group -> global the way the module's own reads do.Configuration::hasKey()inspects one level and would report "absent" under multistore for a value set globally or on the shop group.SELECT name, value FROM ps_configuration WHERE name LIKE 'CONTACTFORM%'returns nothing, then install the module. Before this change the query still returns nothing and submitting the front-office contact form sends no mail at all; after it, the two keys exist as1and0and the merchant receives the notification while the visitor gets no confirmation. Then set the two settings by hand to the opposite values and reset the module: the values must survive, which is what separates this from a plainupdateValue()ininstall().Measured on dev (4.4.3 shop, module reinstalled through
bin/console prestashop:module)Only
install()was varied; the module version was left alone so it is not a second variable.The naive form of this fix - two bare
Configuration::updateValue()calls - was measuredfirst and rejected: a reset flipped the merchant's
notif=0, conf=1back to1, 0,silently overriding both choices.
Two other things the measurement settled:
Configuration::updateValue($key, false)stores SQLNULL, not'0', becauseupdateValue()inserts with$null_values = trueandpSQL(false) === ''(
classes/db/Db.php:444). The seeds use1/0, matching what the settings formitself writes through
Tools::getValue().(
if ($sendNotificationEmail)->Mail::Sendto$contact->email, thenif ($sendConfirmationEmail)->Mail::Sendto the visitor), so1/0really doessend the merchant's mail and not the visitor's.
Shape follows
ps_contactinfo::install(), which already chainsConfiguration::updateValue('PS_CONTACT_INFO_DISPLAY_EMAIL', 1)into its&&return.Design decision
The 2019 thread did not converge: the request was to turn both mails on, and the
objection was that mailing the visitor's typed address makes the shop a relay. Splitting
them resolves it on merits rather than picking a side - the notification carries the
abuse objection not at all, and the confirmation carries all of it.
Issue is labelled
Feature/Needs Specs, but a form that accepts input and notifiesnobody is a defect on its own terms; that is what was measured, and the fix is scoped to
it rather than to the broader settings discussion.
Gate
.php_cs.dist: exit 0; base file exit 0 as control.github/workflows/phpstan/autoload.php: 2 errors,base also 2, 0 new
install(). My own open Tell the customer when a duplicate message was not sent #103 has the exactsame one-file file set but sits at
sendMessage()(~line 666); no hunk overlap.