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

API4: add SQL Function DAYOFWEEK #26063

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Apr 14, 2023

Overview

Adds day of week functions as SQL transformation. Let's you do things like 'find events on Wednesdays'

Before

No day of week functions

After

Day of week functions.

Technical Details

The SQL function returns 1-7, mapped to day names by the 'getOptions()' function.

Comments

Has test!

@civibot
Copy link

civibot bot commented Apr 14, 2023

(Standard links)

@civibot civibot bot added the master label Apr 14, 2023
@colemanw
Copy link
Member

Hey @aydun this looks useful, thanks!
My one pushback would be to add 1 function instead of 2. The presence of the getOptions() means that numeric values will be returned by the API but will be displayed in SK as the name of the day. See SqlFunctionMONTH which follows that pattern.

@@ -234,6 +234,8 @@ public function testDateFunctions() {
->addSelect('QUARTER(birth_date) AS quarter')
->addSelect('MONTH(birth_date) AS month')
->addSelect('EXTRACT(YEAR_MONTH FROM birth_date) AS year_month')
->addSelect('DAYOFWEEK(birth_date) AS day_number')
->addSelect('DAYNAME(birth_date) AS day_name')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->addSelect('DAYNAME(birth_date) AS day_name')
->addSelect('DAYOFWEEK(birth_date):label AS day_name')

Should do the same thing right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - that produces:

There was 1 error:

1) api\v4\Action\SqlFunctionTest::testDateFunctions
CRM_Core_Exception: Aliasing field names is not allowed, only expressions can have an alias.

@aydun
Copy link
Contributor Author

aydun commented Apr 15, 2023

Hi @colemanw I agree it would be better just to have the one function, but how can I get the day name to show in SK results? The names show up in filters and where clauses, but not search results

@colemanw
Copy link
Member

You're right @aydun that was missing. This fixes it #26065

@aydun aydun changed the title Dayofweek API4: add SQL Function DAYOFWEEK Apr 17, 2023
@aydun
Copy link
Contributor Author

aydun commented Apr 17, 2023

Hey @colemanw, that's great - I've updated this just to use the one function. Tests won't pass until #26065 is merged.

@colemanw
Copy link
Member

retest this please

@colemanw
Copy link
Member

@aydun I think this needs a git pull --rebase origin master

@aydun
Copy link
Contributor Author

aydun commented Apr 17, 2023

OK, let's see if that gets the tests to pass....

@seamuslee001
Copy link
Contributor

ok this seems to be passing tests now and Coleman has reviewed this so seems fine to me

@seamuslee001 seamuslee001 merged commit 96a833b into civicrm:master Apr 20, 2023
@aydun
Copy link
Contributor Author

aydun commented Apr 20, 2023

Thanks @seamuslee001

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

Successfully merging this pull request may close these issues.

3 participants