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.

Sign up for GitHub

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

Jump to bottom

dev/core#5725 ScheduledCommunications - Increase length of fields used by SearchKit #32054

Merged
merged 1 commit into from
Feb 12, 2025
Merged

dev/core#5725 ScheduledCommunications - Increase length of fields used by SearchKit #32054

merged 1 commit into from
Feb 12, 2025

Conversation

colemanw
Copy link
Member

Overview

Fixes dev/core#5725

@civibot civibot
Copy link

civibot bot commented Feb 11, 2025

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Feb 11, 2025
@civibot civibot
Copy link

civibot bot commented Feb 11, 2025

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/5725

highfalutin
highfalutin suggested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@highfalutin highfalutin left a comment

Choose a reason for hiding this comment

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

Thank you @colemanw! Could be merged as-is but I have a question and suggestion.

schema/Core/ActionSchedule.entityType.php Outdated
@@ -152,7 +152,7 @@
],
'start_action_date' => [
'title' => ts('Start Action Date'),
'sql_type' => 'varchar(64)',
'sql_type' => 'varchar(255)',
Copy link
Contributor

Choose a reason for hiding this comment

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

This column gets used for SearchKit-generated field aliases, which can get quite lengthy. 255 will probably be long enough, but why chance it? I was thinking something like 2048.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I've updated it.

CRM/Upgrade/Incremental/php/SixOne.php Outdated
'title' => ts('Start Action Date'),
'sql_type' => 'varchar(255)',
'input_type' => 'Select',
'description' => ts('Entity date'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal (it's just the upgrader), but why 'Entity date' instead of 'Start Action Date'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure. All these columns are multipurpose depending on the entity selected so maybe this was trying to be super generic.

@colemanw colemanw force-pushed the alterActionScheduleLength branch from fca3de8 to ee34152 Compare February 11, 2025 19:20
@colemanw colemanw requested a review from highfalutin February 11, 2025 19:24
@eileenmcnaughton
Copy link
Contributor

I might have targetted 6.0 for this - although it seems to be not technically a regression if feels like it 'should' work in 6.0

@colemanw colemanw changed the base branch from master to 6.0 February 11, 2025 20:56
@civibot civibot bot added the 6.0 label Feb 11, 2025
@colemanw colemanw force-pushed the alterActionScheduleLength branch from ee34152 to d726b06 Compare February 11, 2025 20:56
@civibot civibot bot removed the master label Feb 11, 2025
@colemanw colemanw force-pushed the alterActionScheduleLength branch from d726b06 to 4040cc6 Compare February 11, 2025 20:57
@colemanw
Copy link
Member Author

@eileenmcnaughton ok rebased

@colemanw
Copy link
Member Author

@civicrm-builder retest this please

@highfalutin
Copy link
Contributor

E2E_CivicrmAdminUi_ManageGroupsTest.testManageGroups failed with another PR in the same hour and doesn't seem connected to the contents of this PR.

The other failure is E2E\AfformMock\MockPublicFormBrowserTest.testUpdateMiddleName which doesn't seem connected either.

@eileenmcnaughton
Copy link
Contributor

agree those are unrelated

@eileenmcnaughton eileenmcnaughton merged commit d8b9e2d into civicrm:6.0 Feb 12, 2025
1 check failed
@eileenmcnaughton eileenmcnaughton deleted the alterActionScheduleLength branch February 12, 2025 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@highfalutin highfalutin highfalutin approved these changes

Assignees
No one assigned
Labels
6.0 merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colemanw @eileenmcnaughton @highfalutin