-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: osi list shows officials' name with email as fallback #137
Conversation
preload: [:signed_in_employee, :signed_in_by_user], | ||
order_by: [desc: :signed_in_at] | ||
join: u in assoc(si, :signed_in_by_user), | ||
preload: [:signed_in_employee, signed_in_by_user: u], |
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.
suggestion (non-blocking): If you're already in here turning the preload for signed_in_by_user
from the default (which uses a separate query - see here) to using an explicit join in the same query, it might be worth doing the same for signed_in_employee
, thus turning this whole operation into a single query.
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.
ah! thanks, I see you did that in #103. why is that not the default preload behavior, though? I guess because Ecto wants joins to remain explicit?
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.
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.
why is that not the default preload behavior, though?
I'm not sure - it's also possible that Ecto isn't able to generate the join statements fully programmatically in all cases or something? In any event it's just an interesting quirk to be aware of.
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.
One suggestion for a further improvement you could make but overall looks good to me. 👍
…er than separate query
Asana Task: 📐 Orbit: In-app view of completed sign-ins does not include official full name
Display officials' names in the list, with email address as fallback.
Checklist
(x)
Has tests( )
Doesn't need tests( )
Tests deferred (with justification)(x)
Okayed the plan for the feature (e.g. the design files, or the Asana task)( )
Reviewed the feature as implemented (e.g. on dev-green, or saw screenshots)( )
No review needed