Add "Forgot Password" reset feature using Postmark

[jlvallelonga]

adds forgot password flow to the app and utilizes Postmark

[jlvallelonga]

still need to move some things around a bit, but this is the general idea

[krschacht]

Sorry I didn't get to this yesterday. I started today and got through a chunk, and I'm going to an appointment now. I'll go ahead and submit what I got since I don't know when you're working.

One general comment: let's use double quotes everywhere so we don't have the rails single-quote sometimes and double-quotes with interpolation. I've been trying to default to those. And that reminds me, there is an .editorconfig which I think you need to activate in your IDE (and I can't recall, but I think this might do the double quotes automatically, not sure).

[krschacht]

I finished doing a quick pass through the whole PR today. Tag me when you think it's worth me doing another review that way I won't keep jumping in and out with each comment you add. I think it'll be more efficient for me to wait until you've finished your next round of stuff.

But do tag me / chat me if you run into something in the meantime that you want my thoughts on.

[jlvallelonga]

sadly the stub_features and stub_settings don't seem to be able to reload the code that is run in the initializers so I added this back temporarily. I'd like to create a custom Config lib or something that we can use to load settings and features into during init and then mock values of during tests. I hate to make it so complex though. Maybe there's an easier way that I haven't found yet. Will keep digging, but I feel like something like that should come in a separate PR since this has already been open for a while. Thoughts?

[krschacht]

That makes sense since initializers are so early in the app boot. At least the Feature is available in the initializer. I initially thought that was what you were suggesting and ran a quick test myself locally and Feature is definitely working with initializers, so that's good.

I think you're on the right track of basically fixing this within the test environment, but I don't think the CI script is the place to do it because we want someone to be able to pull down the repo locally, run tests, and have them work without needing to set special ENV. I think the right strategy is to somehow we need to change the options.yml defaults just for the test environment.

How about this PR I just tee'd up: #479

If I'm fully understanding the problem, I think this PR will allow you to simply add test_override: "some value" within config.yml to make the test environment work.

[jlvallelonga]

#479 will definitely fix the issue with the incorrect values. I think the problem that I'm running into is not being able to change the values that are dynamically loaded in the application.rb config file and the initializers after the app has started. This means we can't modify one of the features or settings during a test and check that the dependent values (application.config.whatever) is set and more importantly we're unable to test the logic that depends on those dependent values. For instance, some of the password reset logic is dependent on config.password_reset_token_purpose existing which is set dynamically based on the password reset feature being enabled. I can stub the feature so that it's enabled, but that doesn't rerun the initializer code that set the config.password_reset_token_purpose (and whatever else) in the first place. I might be able to just modify the config value at runtime too, but I don't think that's the real solution either because I would have to correctly duplicate the logic from the initializer code to set up the app config values the same way they would be set automatically when the feature env vars are present. Having defaults for the test env will get us set up to run the test with a specific combination of settings/features, but won't allow us to change those env vars during a test and then expect the init code to run and change our app config correctly.

Eh that's a long explanation. Ultimately I think the solution is to not do any dynamic logic in the initializers and move that to a Config class or something if we need to have dynamic logic for a config value. That way we can stub the Config methods. I'd say we can keep it simple and just avoid any dynamic logic during init, but I'm not sure we can hold that off forever given that we're aiming at letting people set this up with different service providers for email, etc.

[krschacht]

Yea, I follow you. Basically, now that we can set different defaults for the test environment options.yml than for the dev/production defaults we should be able to get a test suite that can pass. But you’re right that as soon as we want to have automated tests for postmark and some other service, now we’re back to needing to change the Feature flags within a test.

You’re right that it probably means some way that we move the conditionals outside of initializers, but the problem is that we’re setting rails.application.config values. I’m not too familiar with the inner workings of those.

But it’s also possible that when we do implement another mail provider, it may be trivially different such that there isn’t much additional to test. After all, we’ll want every mail provider to work 99% the same.

I’m okay crossing that bridge when we come to it. We can just defer solving this problem in a deeper way.

updated

[jlvallelonga]

system tests are failing with this error:

Error:
Authentications::PasswordTest#test_should_login_as_an_existing_user_and_logout:
Net::ReadTimeout: Net::ReadTimeout with "Net::ReadTimeout with #<TCPSocket:(closed)>"
    test/system/authentications/password_test.rb:6:in `block in <class:PasswordTest>'

I made another PR with just a change to the readme and it failed in the same way (#481 - was troubleshooting so now it has more than just changes to the readme)

Just updating about where I'm at. Will keep digging

[krschacht]

Dang. I got this CI failure the other day on my PR and I thought it was an issue with github infrastructure. I ran tests 3 times and hit the error each time. Then in the morning I reran the failed tests and it passed. But I just tried re-running your test suite now and it failed again. Out of curiosity, I went to main (where tests had passed) and re-ran the test suite there and it also failed. I’ve found multiple mentions of this issue online and it’s not encouraging. This sounds complex without a great root cause fix. Rob — Do you have any ideas about this error? This thread was a good discussion of the issue but no resolution, and it’s many years old: SeleniumHQ/docker-selenium#198 This issue seemed to explain the root cause is that something during initialization is taking more than 60 seconds: teamcapybara/capybara#1305 (comment) <teamcapybara/capybara#1305> This person suggests that we might want to precompile assets: https://stackoverflow.com/questions/57546441/net-readtimeout-occurs-in-rails-systemtest I found a few mentions online with no useful discussion.

…

Message ID: ***@***.***>

[krschacht]

Over in this PR #483 I tried pre-compiling assets and that didn't fix it. I also tried adding a rescue and retry around login_as, since I noticed that's where it was often failing, but the very next time I ran it it failed on a visit ... line.

[krschacht]

Hi @jlvallelonga where do we stand on this PR? I see tests are passing so we should be really close. I just checked the diff quickly and I see a couple open comments so I didn’t look through the whole thing (e.g. reverting the CI changes and removing the README addition b/c of that other PR). Let me know if you have other to-do’s on this or if it should be done.

[jlvallelonga]

Hi @jlvallelonga where do we stand on this PR? I see tests are passing so we should be really close. I just checked the diff quickly and I see a couple open comments so I didn’t look through the whole thing (e.g. reverting the CI changes and removing the README addition b/c of that other PR). Let me know if you have other to-do’s on this or if it should be done.

Ah sorry. I missed removing that section from the readme. I removed it and the env vars. I'll take another look when I get home tonight, but I think I've got everything now. Ready for merge when you're comfortable with it

[krschacht]

I gave it another once over and it all looks great. Merging in now.