Fix RegistrationTest, account for case with terms feature on

[drehimself]

If you turn on the Features::termsAndPrivacyPolicy() feature in Jetstream, the RegistrationTest doesn't account for this case and will fail. Fixed by passing through the terms field if the feature is on.

[ArnaudLier]

Ses #595

[driesvints]

Hey @drehimself. The test suite is meant to mimic an app in its current working state. It doesn't makes sense to adopt an configurable state in the test suite since you're app will always either have this enabled or disabled. You own this file so feel free to adjust it if you turn on the terms feature.

[Illizian]

@driesvints

Hey @drehimself. The test suite is meant to mimic an app in its current working state. It doesn't makes sense to adopt an configurable state in the test suite since you're app will always either have this enabled or disabled. You own this file so feel free to adjust it if you turn on the terms feature.

Could you clarify what you mean by this? There already exists a range of tests that are toggled by Feature Flags! (See. test_api_token_permissions_can_be_updated, test_email_verification_screen_can_be_rendered), I'm glad I spotted this issue before adding my own PR but it'd be good to know so as to avoid wasted effort in the future.

[driesvints]

@Illizian we started accepting the PRs with feature flags recently because we got tired of having to explain over and over and closing PRs.

[Illizian]

Cool, I'll pop a PR in shortly then 😏😋

…

On Tue, 26 Jan 2021, 12:43 Dries Vints, ***@***.***> wrote: @Illizian <https://github.com/Illizian> we started accepting the PRs with feature flags recently because we got tired of having to explain over and over and closing PRs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFL2OEQ5A57BZAPGVCK3ULS322HRANCNFSM4VZ4UJGQ> .

[driesvints]

These still don't make any sense. Your app will either be in one state or the other. Never in an ability to toggle between states. Just delete/add what you need or don't need.

[Illizian]

Yeah, fair point. It's a weird concept that the tests are provided IMO. I'm still not sure if jetstream is a package or a boilerplate - if that makes sense? It kinda skirts both realms. I came here off the back of existing skipped tests (FWIW).

…

On Tue, 26 Jan 2021, 13:01 Dries Vints, ***@***.***> wrote: These still don't make any sense. Your app will either be in one state or the other. Never in an ability to toggle between states. Just delete/add what you need or don't need. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFL2OAYWHY66HF2XEH5NZ3S324L7ANCNFSM4VZ4UJGQ> .

[driesvints]

@Illizian it's a scaffold tool that mostly scaffolds your app and provides a few internal classes to handle those parts out of the box. But all of the UI is modifiable after it's scaffolded.

[Illizian]

Ok, thanks for the clarification! I rest my sarcasm haha! I didn't realize the existing skips were added under duress and it makes sense if you position jetstream as a boilerplate. There's still some vendor publishes etc that pull it back into package territory though.

…

On Tue, 26 Jan 2021, 13:08 Dries Vints, ***@***.***> wrote: @Illizian <https://github.com/Illizian> it's a scaffold tool that mostly scaffolds your app and provides a few internal classes to handle those parts out of the box. But all of the UI is modifiable after it's scaffolded. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#609 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFL2OBVWUAUAL2RZWTHICDS325FFANCNFSM4VZ4UJGQ> .

[driesvints]

Yeah, we can't really stop that from happening I guess :-/