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

add scopes to generators #6102

Merged
merged 18 commits into from
Mar 6, 2025
Merged

add scopes to generators #6102

merged 18 commits into from
Mar 6, 2025

Conversation

SteffenDE
Copy link
Contributor

@SteffenDE SteffenDE commented Feb 20, 2025

Adds a new concept called "scopes" to the Phoenix generators, as introduced by @chrismccord a while ago:

https://youtu.be/Ckgl9KO4E4M?si=TKgdWCQ87OIPzW_A&t=307
https://github.com/chrismccord/todo_trek/blob/main/lib/todo_trek/scope.ex

@SteffenDE SteffenDE force-pushed the sd-scope branch 2 times, most recently from 3b08167 to 33fbb4c Compare February 20, 2025 16:13

"""
def list_<%= schema.plural %>(%<%= inspect scope.alias %>{} = <%= scope.name %>_scope) do
Repo.all(from <%= schema.singular %> in <%= inspect schema.alias %>, where: <%= schema.singular %>.<%= scope.schema_key %> == ^<%= scope.name %>_scope.<%= Enum.join(scope.access_path, ".") %>)
Copy link
Member

Choose a reason for hiding this comment

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

We just added all_by to Ecto. Perhaps we should do a new release?

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, not the topic of this PR, we can update this and the auth generators to use Repo.transact once Ecto is actually out. I will start working on that front.

@SteffenDE SteffenDE force-pushed the sd-scope branch 2 times, most recently from 2c4172e to e5919e6 Compare February 24, 2025 13:49
apply suggestions from review
@SteffenDE SteffenDE added this to the v1.8 milestone Feb 26, 2025
guides/scopes.md Outdated

> **Requirement**: This guide expects that you have gone through the [introductory guides](installation.html) and got a Phoenix application [up and running](up_and_running.html).

> **Requirement**: This guide expects that you have gone through the [Contexts guide](contexts.html).
Copy link
Member

Choose a reason for hiding this comment

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

Btw, I'd remove those requirements, once we make this more of a reference.

@SteffenDE
Copy link
Contributor Author

Scopes guide updated!

@SteffenDE SteffenDE marked this pull request as ready for review March 3, 2025 15:30

## `on_mount` arguments

* `:mount_current_<%= schema.singular %>` - Assigns current_<%= schema.singular %>
* `:mount_current_scope` - Assigns current_scope
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this mirror fetch_current_scope_for_... and become mount_current_scope_for_...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can change it, but since LV hooks don't have naming conflicts (you always need to pass the module), it's not really an issue

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, sounds good to me!

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

I have added one comment and we can ship it! I will work on the docs this or the next week, so we can ship the RC.

@@ -229,7 +233,7 @@ defmodule <%= inspect auth_module %> do
they use the application at all, here would be a good place.
"""
def require_authenticated_<%= schema.singular %>(conn, _opts) do
if conn.assigns[:current_<%= schema.singular %>] do
if conn.assigns.current_scope do
Copy link
Member

Choose a reason for hiding this comment

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

We need to check current_user.user here, no? For example, you could have an assigned scope that is for a guest (no user), so feels like this check needs to be more strict

Copy link
Member

Choose a reason for hiding this comment

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

Agreed on being more precise here in case someone adds more fields to the scope in the future. It doesn’t hurt.

@josevalim josevalim merged commit bcc984d into main Mar 6, 2025
10 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim josevalim deleted the sd-scope branch March 6, 2025 18:17
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