Skip to content

feat: use scopes with other columns besides created_at #25

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

Merged
merged 6 commits into from
Apr 24, 2025

Conversation

aymanalhattami
Copy link
Contributor

This PR introduces support for applying date scopes on custom columns, not just the default created_at, allowing for greater flexibility when using this package.

Many applications involve models with multiple date fields, and limiting scopes to created_at restricts their usage. This update allows developers to:

  • Reuse existing scopes for various date fields.
  • Simplify querying models by any relevant date column without redundant code.

Example
Transaction::ofToday(column: 'approved_at')

@Sairahcaz
Copy link
Member

Hi @aymanalhattami, thanks for your contribution. I'm not really happy with the API though.
Transaction::ofToday(column: 'approved_at') does not feels like the Laravel way. I would like to keep the parameters as low as possible.

As you might know, there are two ways to tell the package which column to use for the date range queries:

$createdColumnName = (self::CREATED_AT != 'created_at') ? self::CREATED_AT : config('date-scopes.created_column');

First it checks if the model has a custom CREATED_AT constant defined, if not it uses the default value of the config config('date-scopes.created_column').

I would maybe move away from the CREATED_AT constant in the model and rename it to DATE_COLUMN and also rename it in the config to date-scopes.date_column (replace all occurrences in the code). This way you can just tell each model which column it should use for querying...

This would introduce a breaking change and we would need a small migration guide. But it should be pretty simple to implement. I guess it would be simpler to create a new PR for that, instead of reverting your changes...

What do you tink? Would that work for your needs?

@aymanalhattami
Copy link
Contributor Author

aymanalhattami commented Apr 17, 2025

This package is great but right now is limited to only one column in the the table which is the custom column in the model's CREATED_AT property or the one in the config file

First it checks if the model has a custom CREATED_AT constant defined, if not it uses the default value of the config config('date-scopes.created_column').

I tried this configuration, but i need to use the same scopes with more datetime columns in the same table


I would like to keep the parameters as low as possible.

It's nice to keep parameters as low as possible, but from my point of view, it's not right to limit it to only one column in the table.


I would maybe move away from the CREATED_AT constant in the model and rename it to DATE_COLUMN and also rename it in the config to date-scopes.date_column (replace all occurrences in the code). This way you can just tell each model which column it should use for querying

I will make PR and it will be better if it's a method instead of property in order to add the ability to make some logic when necessary


Would that work for your needs?

My PR does not introduce breaking changes, and it still uses the column in the configuration file or in the CREATED_AT model property. what I did is making the package more usable with other datetime columns in the same table and that what works with my needs

Copy link
Member

@Sairahcaz Sairahcaz left a comment

Choose a reason for hiding this comment

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

Hi @aymanalhattami, gotcha - thanks for your reply. Except for the one point, the PR looks good to me. When it's "fixed" we can merge it.

@Sairahcaz Sairahcaz merged commit 4fe4fc8 into laracraft-tech:main Apr 24, 2025
1 check passed
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.

2 participants