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

Phase 2 configuration #15

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Phase 2 configuration #15

wants to merge 4 commits into from

Conversation

bpurinton
Copy link
Contributor

For the AppDev 2 content, we want to use postgres DB. This needs to be in conjunction with this linear ticket, but that will take some more time: https://linear.app/firstdraft/issue/FIR-524/switch-rails-projects-to-postgres-and-implement-actual-db-schema-gem

@bpurinton bpurinton closed this Sep 12, 2023
@bpurinton bpurinton reopened this Sep 12, 2023
@bpurinton
Copy link
Contributor Author

@jelaniwoods @raghubetina I'm having trouble envisioning how we have only one Rails 7 templated for both "Phase 1" and "Phase 2"; while the database configuration could be merged in theory by switching all Phase 1 projects to use Postgres, the other configurations which you can see in e.g. config/application.rb are specific to the two different phases. Also, we might like to remove the draft_generators gem in Phase 2. Among other things I haven't thought all the way through.

It seems kind of unavoidable to have two templates. Given these few differences alone, rather than needing to remember to change the configuration settings every time one sets up a new either Phase 1 or Phase 2 project.

Thoughts? Should we have two templates and name them something like rails-7-template-phase-1 and rails-7-template-phase-2? That's a bit long, but I appreciate a descriptive repository name.

@jelaniwoods
Copy link
Contributor

I might be missing some context, but I thought after we determined the each "phase" needed different settings we concluded that we do need two templates, but unlike before they can share the same Docker image– which was the big time saver.

@raghubetina
Copy link
Contributor

raghubetina commented Sep 13, 2023

Hmm, the only setting I see that is incompatible is:

config.action_controller.default_protect_from_forgery = false

(The other one that we used to have but no longer have is:

config.active_record.belongs_to_required_by_default = false

)

Are there any other incompatibilities?

(I think it's okay to include draft_generators, the gem actually has other generators like draft:scaffold and draft:devise that need updating but are useful post Phase 1. Are there any other gems that we don't want post Phase 1?)

Perhaps we can figure out a workaround if protect_from_forgery is the only thing.

@jelaniwoods
Copy link
Contributor

I'm certain that some other settings are being handled by appdev_support

AppdevSupport.config do |config|
config.action_dispatch = true
config.active_record = true
config.pryrc = :minimal
end
AppdevSupport.init

There might be other settings from this commit that still need to be handled.

@bpurinton bpurinton changed the title postgres db configuration Phase 2 configuration Sep 13, 2023
@bpurinton
Copy link
Contributor Author

bpurinton commented Sep 13, 2023

@jelaniwoods @raghubetina

There might be other settings from this commit that still need to be handled.

You beat me to that yes, I was just pushing a fresh commit on this PR that included what I think are the important configurations to change for Phase 2 projects, where we "take off the training wheels". I guess it's not as bad as I thought, but still requires some thought when generating a new project.

Did I miss anything in the changed files?

config/application.rb Outdated Show resolved Hide resolved
config/initializers/appdev_support.rb Outdated Show resolved Hide resolved
@raghubetina
Copy link
Contributor

raghubetina commented Sep 13, 2023

Hmm. If the Phase 2 gems/settings are a strict subset of Phase 1's, then maybe we could separate the settings into two gems? Then for Phase 2 students (or us when creating Phase 2 projects from the template), disabling the "training wheels" would be a matter of removing one gem from the Gemfile, but the long-term helpful settings could be kept?

@bpurinton
Copy link
Contributor Author

Hmm. If the Phase 2 gems/settings are a strict subset of Phase 1's, then maybe we could separate the settings into two gems?

I think we can put a pin in this conversation for now, if my current updates on this branch seem to cover basic config difference for Phase 2 projects. This came up because I'm currently trying to fill in the DPI canvas and on the Phase 2 content where I realized there were some setting differences.

If this PR looks good at the moment, I can just use it for reference for now to generated those few phase 2 projects (there aren't many, mostly helper methods and photogram industrial)

@bpurinton
Copy link
Contributor Author

bpurinton commented Sep 13, 2023

@jelaniwoods I'm having an issue with forgery protection in Codespaces; locally, I set

# config/application.rb

config.action_controller.default_protect_from_forgery = true

And it works fine, requiring a hidden authenticity token in the POST form.

But in this app (https://github.com/appdev-projects/helper-methods-part-1-and-2) in a GitHub codespace; I get the server log error:

ActionController::InvalidAuthenticityToken - HTTP Origin header (http://localhost:3000) didn't match request.base_url (https://sturdy-space-sniffle-g4xxvv4grvrhvp6x-3000.app.github.dev):

When I use the new movie form.

I'm not sure the best way to solve this / if it is a Docker image issue?

Edit: Turns out I can get it working by adding:

# config/application.rb

config.action_controller.forgery_protection_origin_check = false

But is that a bad idea? i.e., Would that be dangerous if someone deployed to production? It looks like this is an additional line of CSRF defense

@jelaniwoods
Copy link
Contributor

But is that a bad idea? i.e., Would that be dangerous if someone deployed to production? It looks like this is an additional line of CSRF defense

I don't remember running into this issue with vanilla-rails in Gitpod, so it might be a codespaces bug. I found a related issue which appears to confirm your solution as the only viable work-around.

It should be in development.rb instead though, since we would want forgery protection enabled in production.

@bpurinton
Copy link
Contributor Author

It should be in development.rb instead though, since we would want forgery protection enabled in production

@jelaniwoods Aha, nice! Thanks. That solves the issue, see my latest commit. This seems like one thing that should actually be added to main on the template?

@jelaniwoods
Copy link
Contributor

This seems like one thing that should actually be added to main on the template?

Yes, you're right. 🤔 It's still odd to me that this is required at all, since I remember doing significant testing on this template to make sure everything worked in Codespaces. I guess it could be something recent with Codespaces and/or Rails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants