Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🥗🧹 Gizmo: System Spec to Add and Remove Gizmo #1588

Merged
merged 2 commits into from
Jun 25, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Jun 22, 2023

For some reason the automated check doesn't FAIL despite it failing when we manually execute it!

The "cause" is that the gizmo form has two _method fields, one is the hidden _method field that is automatically inserted by form_with when the model is persisted?; the other is added by a button that sets the _method to delete so that it can be removed.

In the system spec; the driven firefox takes the _method set by the clicked button. In the user-land; it sends both methods; and Rails picks the first one.

The "solution" will be to ditch the second _method (possibly even removing the ability to Remove a Gizmo from the update form.)

@zspencer zspencer marked this pull request as draft June 22, 2023 02:09
@zspencer zspencer force-pushed the 1565/write-system-test branch 4 times, most recently from d344c03 to c3c21dc Compare June 25, 2023 18:40
bin/setup Outdated
Comment on lines 51 to 57
if test -f ".env.development"; then
echo "Found .env.development, leaving it in place!"
else
echo "Copying .env.development.example to .env.development"
cp .env.development.example .env
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling the .devcontainer/startup.bash script may need to be updated with this new .env changes?
Specifically, the block starting at line 92 may need to be adjusted.
This assumption may be wrong, but I wanted to bring it up sooner rather than later.

Copy link
Member Author

@zspencer zspencer Jun 25, 2023

Choose a reason for hiding this comment

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

Great catch!!!

It would be nice if the three places we use (bin/setup, bin/setup-rails, and .devcontainer/startup.bash) all used the same shell script to create these default ffiles; perhaps bin/setup-env? Are you available to do that refactor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I can do that.

One script to do a conditionally copy of both .env.example and .env.development.example?
And then in the .devcontainer/startup.bash script, do the sed stuff after running this script?
Mostly just probing to make sure I'm understanding what needs to be here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! I think the sed stuff should stay in the startup.bash

@zspencer zspencer marked this pull request as ready for review June 25, 2023 19:36
@zspencer zspencer requested review from a team June 25, 2023 19:36
.env.example Outdated
# SMTP_DOMAIN=localhost
# SMTP_AUTHENTICATION=plain
# SMTP_ADDRESS=localhost
# SMTP_ENABLE_TLS=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break out system test step to it's own PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Plz review!

zspencer added a commit that referenced this pull request Jun 25, 2023
zspencer added a commit that referenced this pull request Jun 25, 2023
@zspencer zspencer changed the base branch from main to neighborhood/sprout-system-spec-setup June 25, 2023 19:49
Base automatically changed from neighborhood/sprout-system-spec-setup to main June 25, 2023 19:57
zspencer added a commit that referenced this pull request Jun 25, 2023
- #1565

For whatever reason the automated check doesn't *FAIL* despite it
failing when we manually execute it!

Add Firefox to the RSpec tests

Run in Headless mode on CI

Don't send real emails in CI

save screenshots yo

Add the .env.development to the devcontaine

Add Firefox to the RSpec tests

Run in Headless mode on CI

Don't send real emails in CI

save screenshots yo
- #1565

OK this is also a bit much; but it does fix the bug. I'm going to try
and tighten it up a bit
Copy link
Contributor

@KellyAH KellyAH left a comment

Choose a reason for hiding this comment

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

LGTM

@zspencer zspencer merged commit 4313984 into main Jun 25, 2023
@zspencer zspencer deleted the 1565/write-system-test branch June 25, 2023 20:08
@zspencer zspencer added 🥗 test automation Adds some automated tests. V nutritious. 🧹 refactor Includes non-behavioral changes labels Jul 11, 2023
@zspencer zspencer changed the title 🥗 Gizmo: System Spec to Add and Remove Gizmo 🥗🧹 Gizmo: System Spec to Add and Remove Gizmo Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 refactor Includes non-behavioral changes 🥗 test automation Adds some automated tests. V nutritious.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants