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

limit results to contacts with out district info entered already. #7

Merged

Conversation

jmcclelland
Copy link

No description provided.

@MegaphoneJon
Copy link
Owner

Hi Jamie,

I think this is a move in the right direction, but two questions:

  • Why do we group by contact_id here? In fact, I'm not sure why we're grouping by ID at all - I don't see how this query could return duplicates. That may be something I ported over from the SQL without looking closely.
  • I agree with adding the new addWhere clause - but should we be removing the old one? It seems like this will cause us to redistrict addresses that have errors.

@jmcclelland
Copy link
Author

I changed the group by to contact_id to be extra sure we don't return the same contact twice. I haven't tested but... what if we choose "Home" for the location type and a contact has two home addresses? Without group by, I think we would get two records from them which would eat two credits, but the second lookup would over write the first.

I also think we need to keep the old where clause. If we try a lookup and it fails, then electoral_districts.electoral_level will be NULL since nothing was retrieved and, hopeully, electoral_status.electoral_status_error_code will have a value indicating an error. By keeping that where clause, we ensure that we don't keep trying to re-district the same failing address over and over again.

@MegaphoneJon
Copy link
Owner

My main concern about the group_by is that it will give us an indeterminate electoral status error code in the scenario you describe - but multiple addresses of the same location type aren't really supported in Civi. You can create them with API or import, but then when you re-save one, it overwrites the other. So I guess I'm fine with this since both scenarios can lead to unexpected results, neither is supported, and neither is particularly worse than the other.

@MegaphoneJon MegaphoneJon merged commit 0f9dd8f into MegaphoneJon:master Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants