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

Fix errors when setting custom namespace #80

Closed
wants to merge 5 commits into from

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Oct 6, 2022

Concerning issue 428 in the starter repo

Now that we have multiple routes files, I used routes_path to make sure we're writing to the correct files.

Concerning #78

if cli_options["namespace"]
cli_options["skip-api"] = true
cli_options["skip-model"] = true
cli_options["skip-locales"] = true
end

We have skip-api set to true here, so we can generate the missing API controller in #78 by removing this line. I'm assuming that's what we'll want to do since we're already generating the API views when super scaffolding. Things have changed a lot since we stopped using Grape, so I can see us removing the flag here.

Concerning the missing show page, here's the block that tries to add a child to the parent's show page:

# add children to the show page of their parent.
unless cli_options["skip-parent"] || parent == "None"
scaffold_add_line_to_file(
"./app/views/account/scaffolding/absolutely_abstract/creative_concepts/show.html.erb",
code_for_child_on_parent_show_page || "<%= render 'account/scaffolding/completely_concrete/tangible_things/index', tangible_things: @creative_concept.completely_concrete_tangible_things, hide_back: true %>",
"<%# 🚅 super scaffolding will insert new children above this line. %>",
prepend: true
)
end

In #78's case, the child is Job. However, the string is getting transformed in such a way that the parent Team is getting namespaced under the custom namespace client:
'./app/views/client/teams/show.html.erb'

Since the namespace client doesn't have its own set of views, do we need to add a list of the children to the Team show page, or can we just skip this part? I'm guessing that we should add it, so I'll try to set this PR as ready for review once I get that set up.

Tests

I don't think we have any tests in place for the namespace option, so I'll keep that on my radar as I work through this one.

@gazayas
Copy link
Contributor Author

gazayas commented Oct 6, 2022

@andrewculver As I'm sifting through this one, I feel like there's a lot of moving parts and I want to make sure I'm headed in the right direction. In other words, I guess I don't have a clear goal in sight as to how these namespaced files need to be set up, and I don't want to start building too much in the wrong direction.

Controller placement

For example, one thing that stands out is the directory hierarchy for the normal child controller. It is namespaced under client, as expected:
app/controllers/client/jobs_controller.rb.

However the API controller is here, not under client:
app/controllers/api/v1/jobs_controller.rb

I'm guessing that we need to create the client directory and put the controller in there.

Routes files

When setting client as a custom namespace, we add a new file client.rb and use draw to include it in the original routes file. However, we don't do anything like this with the API routes file. Should we add a new hierarchy per version which the main API routes file includes using draw as well? For example:

config/
  routes/
    api/
      v1/
        v1.rb
        client.rb
    client.rb
    concerns.rb
    devise.rb
    sidekiq.rb
  routes.rb

Other files

I think I'll wait until we have a clear direction on this part, because I don't want to take up too much time if it's not what we need.

As far as my analysis of the issue goes though, it seems like these are the main things we need to take care of.

@gazayas
Copy link
Contributor Author

gazayas commented Oct 7, 2022

With this last commit, I made sure we're including the custom namespaced routes file (config/routes/client.rb) in our main routes file, because it wasn't being included using draw.

Now that I'm looking over the code, it seems like everything isn't set in place yet (for example, code_for_child_on_parent_show_page which I brought up in the first comment is an empty method added recently in f46d36b), so I think I'll cut the PR here.

I could see us doing the following things next:

  1. Editing paths in the API controller test code to reflect the custom namespaces.
  2. Editing Super Scaffolded controller names to use the correct namespaces.
  3. Adding the API routes hierarchy that I mentioned in the previous comment.
    etc...

I'll leave things as is for now since I'm noticing some other TODOs in the Transformer that I overlooked before.

@gazayas gazayas marked this pull request as ready for review October 7, 2022 05:04
@andrewculver
Copy link
Contributor

@gazayas Can you resolve the conflicts on this one?

@gazayas
Copy link
Contributor Author

gazayas commented Dec 28, 2022

Closing in favor of bullet-train-co/bullet_train-core#35

@gazayas gazayas closed this Dec 28, 2022
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.

Defining custom namespace overwrites routes.rb
2 participants