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

dev/core#1471 - Add system status alert for deleted custom fields in … #16267

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Jan 10, 2020

…smart groups

Overview

Add message alert on system status page for deleted custom fields on smart group.

Before

See https://lab.civicrm.org/dev/core/issues/1471. There is no indication or way to know which smart groups are responsible for errors on the group page.

After

System status show an alert with a list of groups responsible for smart group errors.

image

UPDATED text after QA -

image

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/1471

@civibot
Copy link

civibot bot commented Jan 10, 2020

(Standard links)

@civibot civibot bot added the master label Jan 10, 2020
@francescbassas
Copy link
Contributor

In the alert message, instead of deleting problematic smart groups, I might suggest editing their search criteria and updating them to clean outdated fields from saved search.

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Jan 13, 2020

@francescbassas updated the msg to suggest editing and updating the search criteria. Also updated the table to list custom field labels of deleted fields that are included in the saved search form values.

image

@francescbassas
Copy link
Contributor

francescbassas commented Jan 13, 2020

Great, looks better!

Custom field value isn't displayed in my case. Maybe because they are deleted and not disabled?

screenshot

I would also add a ✏️ and ⚙️ to each row to link to the group search criteria edition .../civicrm/contact/search/advanced?reset=1&ssID=5 or the settings page to deactivate it civicrm/group?reset=1&action=update&id=5.

@yashodha
Copy link
Contributor

@jitendrapurohit @francescbassas Edit/settings button sound good. Can the list be group title which is link to the results as well.

@francescbassas
Copy link
Contributor

In the column Custom field I would put:

  • Deleted (red color) - if field has been removed
  • The name of the field (disabled color with a link to edit group ...civicrm/admin/custom/group/field/update?action=update&reset=1&gid=XX&id=XX) - if field has been disabled

@yashodha
Copy link
Contributor

@francescbassas sounds good. @jitendrapurohit Can you please make the requested changes? Is the test failure related?

@eileenmcnaughton
Copy link
Contributor

I don't think the test failure is related - @yashodha go ahead & merge this once you are happy with it

@eileenmcnaughton
Copy link
Contributor

test this please

@jitendrapurohit
Copy link
Contributor Author

I've updated the PR with the additional requirements. The updated SS is as shown below -

image

}
}

$html .= "<tr><td>{$id}</td><td>{$field['title']}</td><td class='disabled'>{$fieldName}</td>";
Copy link
Contributor

@francescbassas francescbassas Jan 16, 2020

Choose a reason for hiding this comment

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

To add edit and config group icons:

        $groupEdit = '<a href="' . CRM_Utils_System::url('civicrm/contact/search/advanced', "?reset=1&ssID={$id}", TRUE) . '" title="' . ts('Edit search criteria') . '"> <i class="crm-i fa-pencil"></i> </a>';
        $groupConfig = '<a href="' . CRM_Utils_System::url('civicrm/group', "?reset=1&action=update&id={$id}", TRUE) . '" title="' . ts('Group settings') . '"> <i class="crm-i fa-gear"></i> </a>';
        $html .= "<tr><td>{$id} {$groupEdit} {$groupConfig}</td><td>{$field['title']}</td><td class='disabled'>{$fieldName}</td>";

Aso I would convert the 'ID' and 'Group Title' to one 'Group'

proposal

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I put on $groupConfig the group ID when should be the SSID.

@jitendrapurohit
Copy link
Contributor Author

@francescbassas The above change has been made. Can you pls retest and confirm? Thanks.

@francescbassas
Copy link
Contributor

Tested and looks good. Thanks @jitendrapurohit.

final

Looking again, I could just only up the last paragraph above the table.

test this please

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@francescbassas is this good to merge once tests pass?

…smart groups

additional changes from francescbassas
@jitendrapurohit
Copy link
Contributor Author

Looking again, I could just only up the last paragraph above the table.

done @francescbassas

@yashodha
Copy link
Contributor

@jitendrapurohit Can you please update the latest screenshot in After section in which all the changes have been incorporated or is this the one https://user-images.githubusercontent.com/1064675/72603392-37b81d00-3919-11ea-8d93-1bc121f6be74.png?

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Jan 27, 2020

@yashodha Done - #16267 (comment)

@yashodha
Copy link
Contributor

@jitendrapurohit looks good, merging this. Thanks!

@yashodha yashodha merged commit fa6737a into civicrm:master Jan 31, 2020
@eileenmcnaughton
Copy link
Contributor

nice! Thanks all

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.

4 participants