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

Fix Schema System Check translation and show missing Custom Field ID #23469

Merged
merged 1 commit into from
May 17, 2022

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented May 16, 2022

Overview

When the Schema System Check finds a smartgroup using deleted Custom Fields, it just says that the field is deleted. This is not terribly useful, either to fix the saved search, or to do a bit of archaeology using logging tables.

Also, this PR fixes the translation of strings.

Before

image

After

image

@civibot
Copy link

civibot bot commented May 16, 2022

(Standard links)

@civibot civibot bot added the master label May 16, 2022
$html
</tbody></table></p>
";
$message = "<p>" . ts('The following smart groups include custom fields which are disabled/deleted from the database. This may cause errors on group page. You might need to edit their search criteria and update them to clean outdated fields from saved search OR disable them in order to fix the error.') . '</p>'
Copy link
Contributor

@demeritcowboy demeritcowboy May 16, 2022

Choose a reason for hiding this comment

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

This was also there before but I'm not clear on what "errors on group page" means. Maybe it should be "This may cause errors when using the group." Or even just "This may cause errors.". At the very least it should have a "the" in it, but I don't know what "the group page" is referring to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated for now to "This may cause errors when recalculating the smart group counts", because I think that's the most
accurate problem, and it's also the name of the button on the "Manage Groups" page (I'd refer to the name of that section explicitly, but I couldn't find a non-awkward way to say it).

but will circle back

@mlutfy mlutfy force-pushed the schemaTranslation branch from 2b82e45 to 223e630 Compare May 17, 2022 00:28
@mlutfy mlutfy force-pushed the schemaTranslation branch from 223e630 to 1569d6d Compare May 17, 2022 13:23
@mlutfy
Copy link
Member Author

mlutfy commented May 17, 2022

@demeritcowboy Thanks for the review! I updated the string again to this:

The following smart groups include custom fields which are disabled or deleted from the database. Missing fields should automatically be ignored from the smart group criteria, but you may want to review and update their search criteria to remove the outdated fields.

I think it's more accurate this way, because as far as I know, smart groups do not crash when a field is removed, and people should not be too worried about it (but it can be very useful to catch an error such as, removing a field, which ends up altering the meaning of smart-groups).

@demeritcowboy
Copy link
Contributor

I don't want to bog this down too much since this PR wasn't about the wording, but what happens is that the group ends up including way more people. E.g. if you have a smart group that filters on contact type = Student and custom field A = "something", then you disable/delete the custom field, the group now includes ALL students whenever it gets used.

So how about let's merge this to get the field ID and ts fixes, and can think about the wording.

@demeritcowboy demeritcowboy merged commit 436cc99 into civicrm:master May 17, 2022
@demeritcowboy
Copy link
Contributor

One of the extra question marks was still there so have put up #23480

@mlutfy mlutfy deleted the schemaTranslation branch May 17, 2022 15:52
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.

2 participants