Closed (fixed)
Project:
Drupal CMS development repository
Component:
Track: Contact form
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
14 Jun 2024 at 02:01 UTC
Updated:
1 Nov 2024 at 04:24 UTC
Jump to comment: Most recent
Comments
Comment #2
pameeela commentedComment #3
pameeela commentedComment #4
thejimbirch commentedComment #5
pameeela commentedThis track is in need of a lead. See Dries' blog post for more info, read about the track lead position, or just apply now!
Comment #6
rachelh_designWe're attempting to organize a team to work on this at Oomph. For me, I can help with gathering requirements and any design needs. Once we find the right person to lead, we'll send in the application!
Comment #7
pameeela commentedGreat news, thanks @rachelh_design!
Comment #8
pameeela commentedComment #9
pameeela commentedComment #10
pfrillingComment #12
phenaproximaThere are a few things here which need to be changed.
Comment #13
pfrillingI fixed the obvious things and left comments for a few others. Changing this to needs review for visibility, but I'm sure more work will follow.
Comment #14
phenaproximaThank you for the very clear and sensible explanations! I think I can see a few ways forward; commented in the MR. Curious what you think.
Comment #15
pfrillingI think this is ready to review again.
Comment #16
phenaproximaThis issue has an MR needing review, so I guess it's...not a meta. Removing that designation.
Comment #17
phenaproximaThat looks pretty good to me overall, just a few points of clarification/feedback/follow-ups.
Comment #18
phenaproximaEverything looks great here - I just have a suggestion for `drupal_cms_anti_spam` so that we can be certain the `authenticated` user role exists, without merely hoping that it was installed by the User module (even though it's a required role -- we're just being paranoid here).
Once that suggestion is implemented (or kicked back if it doesn't work), I'm okay committing this.
Comment #19
pameeela commentedWe still want a meta, so I created a new one and updated references.
Comment #20
jurgenhaasWith regard to the anti spam integration of the contact form, please note there is a proposal at #3477309: Add the Friendly CAPTCHA module, and optional CAPTCHA support to the contact form to use friendlycaptcha for that. The contact form recipe requires the antispam recipe, so if the antispam changes strategy, this will change for the contact form as well.
Just wanted to leave this heads-up here just so that this recipe doesn't necessarily have to put extra effort into anti-spam, unless you have additional requirements?
Comment #21
pfrillingI addressed all the feedback in the MR.
- I decided to list out all of the webform configuration instead of archiving the one form that shipped with Webform.
- I reverted any changes I made to the anti spam recipe. I'll create a followup issue to address changes in #3477309
- I did add a cypress e2e test, which passes locally, but not in CI. I don't think the E2E testing is configured quite right in the CI. It seems that the site is not getting installed in that CI environment.
Comment #22
phenaproximaThis is shaping up; I love the test coverage! However, there are some flaws here that arise from a few minor misunderstandings of how to write recipes. I'm also very ambivalent about explicitly importing all of Webform's config entities, even the ones we don't need.
Comment #23
pfrillingI believe I addressed everything. This is ready for another review.
Comment #24
phenaproximaTests are failing, and we'll need to fix that before merging -- but I'm also busily building a basic framework for Cypress testing over in #3479610: Add Cypress test to the base SEO recipe, so let's postpone this on that.
Comment #25
phenaproximaAlright - #3479610: Add Cypress test to the base SEO recipe is in, so I think we can start hauling this to the finish line! Let's get the tests passing, and model the E2E tests on
drupal_cms_seo_basic/tests/e2e/spec.cy.js.Comment #26
phenaproximaI think this is good to go. The recipe looks good, is lean, and has strong test coverage (thanks @pfrilling)! We don't need to hold this up any longer. With regard to @jurgenhaas's point in #20, the tests will certainly help us adjust properly if and when we switch to Friendly CAPTCHA.
I'm shipping it!
Comment #27
phenaproximaHmmm...something odd is going on with one test, not sure why - self-assigning to figure it out.
Comment #28
phenaproximaBoy, getting this to work properly took some doing, but it also revealed a major flaw in the way I built some of the Cypress infrastructure in #3479610: Add Cypress test to the base SEO recipe. What can I say...this whole thing is a learning experience. Anyway - that flaw, at least, is fixed and means that specs are properly isolated from each other.
Comment #30
phenaproximaFinally! The ship sails. Great work here!