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

Update phx.gen.auth timestamps to match config #5794

Merged
merged 5 commits into from May 2, 2024

Conversation

rhcarvalho
Copy link
Contributor

Follows up on #5586 that introduced the concept but missed a few places that set timestamps.

Follows up on phoenixframework#5586 that introduced the concept but missed a few places that set timestamps.
All generators seem to separate 'timestamps()' from the field
definitions.
@rhcarvalho rhcarvalho marked this pull request as draft April 28, 2024 18:21
@rhcarvalho
Copy link
Contributor Author

Need to test this better, perhaps the change of data type has unintented impact in other places.

Copy link
Contributor Author

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Manually tested that generated app with these changes passes it's own generated tests. In particular, setting :confirmed_at using the correct type.

@@ -8,7 +8,8 @@ defmodule <%= inspect schema.repo %>.Migrations.Create<%= Macro.camelize(schema.
<%= if schema.binary_id do %> add :id, :binary_id, primary_key: true
<% end %> <%= migration.column_definitions[:email] %>
add :hashed_password, :string, null: false
add :confirmed_at, :naive_datetime
add :confirmed_at, <%= inspect schema.timestamp_type %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would output one of :naive_datetime, :utc_datetime or :utc_datetime_usec.

I understand from the first note in https://hexdocs.pm/ecto/Ecto.Schema.html#module-primitive-types that those are valid values both in the schema definition and in the migration file.

When using database migrations provided by "Ecto SQL", you can pass your Ecto type as the column type. However, note the same Ecto type may support multiple database types. For example, all of :varchar, :text, :bytea, etc. translate to Ecto's :string. Similarly, Ecto's :decimal can be used for :numeric and other database types. For more information, see all migration types.

<% :naive_datetime -> %>now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
<% :utc_datetime -> %>now = DateTime.utc_now() |> DateTime.truncate(:second)
<% :utc_datetime_usec -> %>now = DateTime.utc_now() |> DateTime.truncate(:microsecond)
<% end %>change(<%= schema.singular %>, confirmed_at: now)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only handling the 3 documented values; formatted so as to not produce extraneous empty lines when rendered.

I noticed Elixir 1.15 supports the shorter DateTime.utc_now(:second), but I assume that since phx.new generates a mix.exs requiring Elixir 1.14 then there's no reason to break compatibility right now.

@rhcarvalho rhcarvalho marked this pull request as ready for review April 28, 2024 20:28
@josevalim josevalim merged commit b5c7f5b into phoenixframework:main May 2, 2024
6 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@rhcarvalho rhcarvalho deleted the patch-4 branch May 14, 2024 06:34
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.

None yet

2 participants