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-20964: Include queue id while retrieving rows from Event Queue #10757

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Jul 25, 2017

CRM_Mailing_Event_BAO_Queue::getRows(); return rows for each intended recipients, but doesn't include any info which differentiates each row.

This adds a queue_id in the key column of the $result array which enables hooks to know which row is getting displayed.

It seems safe to add key here as getRows() is only called at this point which doesn't use the key info for manipulation.


@jitendrapurohit
Copy link
Contributor Author

test this please

@lcdservices
Copy link
Contributor

Should the ID be added as an array element too? That would make it clear to developers what the ID is, whereas only using it for the array key is less apparent.

@jitendrapurohit
Copy link
Contributor Author

Thanks for the response @lcdservices. I'll check and update if it doesn't break any stuffs.

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Aug 30, 2017

@lcdservices Sorry for the delay.

Attaching queue id to the array element would result in addition of another column in the display page. Note the id 1 after the date column in the below screenshot -

image

Reason why I included this as a key rather than column, as from the code it seems the key is not being used for any manipulation by civi. And the hooks eg module_civicrm_searchColumns() can easily make use of this queue_id without affecting any core functionality.

@lcdservices
Copy link
Contributor

Ok. I didn't realize the array was used in its entirety for the table construction. Using the ID as the array key is fine and a useful improvement. @monishdeb can you merge this.

@monishdeb monishdeb merged commit db52a21 into civicrm:master Aug 30, 2017
@monishdeb
Copy link
Member

Done

@jitendrapurohit
Copy link
Contributor Author

Thanks @lcdservices @monishdeb

@jitendrapurohit jitendrapurohit deleted the CRM-20964 branch August 31, 2017 02:34
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