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

CRM-21349: Increase timeout of status message after batch merge #11195

Merged
merged 4 commits into from
Nov 11, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Oct 25, 2017

Overview

Increase timeout of status message after batch merge process

Before

Batch merge process usually takes a longer time to complete and displays the status message on completion but it drops off the screen which is no of help as most likely the user won't be watching the screen every second to see when it ends.

After

timeout increased. User can see the message after merge process is complete.


@@ -149,7 +149,7 @@ public function run() {

$stats = CRM_Dedupe_Merger::getMergeStatsMsg($cacheKeyString);
if ($stats) {
CRM_Core_Session::setStatus($stats);
CRM_Core_Session::setStatus($stats, '', 'alert', array('expires' => 0));
Copy link
Member

Choose a reason for hiding this comment

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

How about giving it a title as well, like "Batch Complete". Also, is this a success message?

@colemanw
Copy link
Member

colemanw commented Nov 8, 2017

@jitendrapurohit did you see my question?

@jitendrapurohit
Copy link
Contributor Author

@colemanw Yes, I think I missed the notification. The title is being updated.

For the type - this also includes a message when the contacts were skipped during the merge process, due to which I added alert as a param so that the below status message makes sense.

image

Agree, this looks weird when all the contacts were successful in the merge. So thinking of updating the code to do something like -

$type = preg_match('/skipped/', $stats) ? 'alert' : 'success';

looks good?

@colemanw
Copy link
Member

colemanw commented Nov 9, 2017

That would only work in English. But I'm noticing the getMergeStats function isn't using ts() correctly anyway so I'm pushing an update. Can you please test this @jitendrapurohit?

@jitendrapurohit
Copy link
Contributor Author

Thx for the update. I'll test this during the weekend.

@jitendrapurohit
Copy link
Contributor Author

Just noticed, the plural message wasn't working due to the integer param. It displayed One contact was merged message even when I had >1 number of count. Tested after the final update and it works correctly. Can this be merged @colemanw ?

@colemanw colemanw merged commit 3c8b115 into civicrm:master Nov 11, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21349: Increase timeout of status message after batch merge
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