-
Notifications
You must be signed in to change notification settings - Fork 60
Add fallback for missing head of institution name #580
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
base: main
Are you sure you want to change the base?
Conversation
|
Hey @Ainali, I’ve made the requested updates: -->Split the office (P2388, P1313) and person-based properties (P488, P169, P1037, P3975) into separate blocks. -->Removed the COALESCE fallback, since the frontend handles missing data. Everything should now work as expected. Please have a look when you get a chance. Thanks! |
It looks like you forgot to remove it. |
|
Hi @Ainali — I’ve now removed the COALESCE fallback from ?leadBy as requested. |
|
Hi @Ainali, |
|
@Rahul2322-P Have you tried these changes locally? |
Ainali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked around on a number of countries when running this locally, and it adds a bunch of new lead by correctly, and I cannot see that it breaks anything else either, so I'll approve it but I hope that @Abbe98 could double-check so I am not missing anything.
I would still appreciate an answer if you tried this locally. |
|
Thanks, @Ainali! |
That's odd. You shouldn't even have the rights to merge. |
|
Thanks everyone for reviewing. Everything looks good on my end and the tests confirm expected behavior. I’ll hold off until @Abbe98 gives the final go-ahead. Really appreciate the quick feedback and the labels! |
Abbe98
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming they are in the priority order (left to right, SAMPLE will always pick the first one) we want, I'm okay with this.
Two line breaks are left to be cleaned up, but that's it.
|
Thanks @Abbe98 |
|
Hi @Abbe98 — just following up to check if the cleanup looks good now. |
|
There is still a odd line in the OPTIONAL section. |
Made a small update to improve how we pull the head of an institution:
-->Added a few fallback properties like chair (P488), CEO (P169), director (P1037), and Secretary-General (P3975)
-->Also added a COALESCE so if the name’s missing, it’ll show “Not Available” instead
This should help keep the results meaningful even when the main leader info isn’t there.
Appreciate you taking the time to review it!