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

feat: osi list shows officials' name with email as fallback #137

Merged
merged 2 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion js/components/operatorSignIn/list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const List = ({ line }: { line: HeavyRailLine }): ReactElement => {
.replace(/ /g, "")}
</td>
<td className="fs-mask border-y md:border-x p-1 break-words [hyphenate-character:'']">
{si.signed_in_by_user.replace(
{si.signed_in_by.replace(
/@/g,
// 173 is a soft hyphen, which here acts as a breaking suggestion
// We are hiding the hyphen above with CSS
Expand Down
2 changes: 1 addition & 1 deletion js/models/signin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { z } from "zod";
export const SignIn = z.object({
rail_line: z.enum(["blue", "orange", "red"]),
signed_in_at: z.string(),
signed_in_by_user: z.string(),
signed_in_by: z.string(),
signed_in_employee: z.string(),
});

Expand Down
2 changes: 1 addition & 1 deletion js/test/components/operatorSignIn/list.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jest.mock("../../../hooks/useSignIns", () => ({
signed_in_at: DateTime.fromISO("2024-07-22T12:45:52.000-04:00", {
zone: "America/New_York",
}),
signed_in_by_user: "[email protected]",
signed_in_by: "[email protected]",
signed_in_employee: EMPLOYEES[0].badge,
},
],
Expand Down
2 changes: 1 addition & 1 deletion js/test/hooks/useSignins.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const TEST_DATA = {
{
rail_line: "blue",
signed_in_at: "2024-07-22T16:42:32Z",
signed_in_by_user: "[email protected]",
signed_in_by: "[email protected]",
signed_in_employee: "123",
},
],
Expand Down
26 changes: 19 additions & 7 deletions lib/orbit_web/controllers/sign_in_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,28 @@ defmodule OrbitWeb.SignInController do
^start_datetime <= si.signed_in_at and
si.signed_in_at < ^end_datetime and
si.rail_line == ^rail_line,
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],
Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@lemald lemald Sep 17, 2024

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.

left_join: signed_in_by_employee in Employee,
on: u.email == signed_in_by_employee.email,
order_by: [desc: :signed_in_at],
select: %{
signin: si,
signed_in_by_employee: signed_in_by_employee
}
)
)
|> Enum.map(fn si ->
|> Enum.map(fn i ->
%{
rail_line: si.rail_line,
signed_in_at: DateTime.to_iso8601(si.signed_in_at),
signed_in_by_user: si.signed_in_by_user.email,
signed_in_employee: si.signed_in_employee.badge_number
rail_line: i.signin.rail_line,
signed_in_at: DateTime.to_iso8601(i.signin.signed_in_at),
signed_in_by:
if i.signed_in_by_employee do
Employee.display_name(i.signed_in_by_employee)
else
i.signin.signed_in_by_user.email
end,
signed_in_employee: i.signin.signed_in_employee.badge_number
}
end)
}
Expand Down
43 changes: 42 additions & 1 deletion test/orbit_web/controllers/sign_in_controller_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ defmodule OrbitWeb.SignInControllerTest do
%{
"rail_line" => "blue",
"signed_in_at" => _date,
"signed_in_by_user" => _user,
"signed_in_by" => _user,
"signed_in_employee" => _badge
}
]
Expand Down Expand Up @@ -64,6 +64,47 @@ defmodule OrbitWeb.SignInControllerTest do
} = json_response(conn, 200)
end

@tag :authenticated
test "shows officials' names if available", %{conn: conn} do
insert(:employee, email: "[email protected]")

insert(:operator_sign_in,
signed_in_at: DateTime.new!(~D[2024-07-21], ~T[12:00:00], @timezone),
signed_in_by_user: build(:user, email: "[email protected]")
)

conn = get(conn, ~p"/api/signin", %{"line" => "blue", "service_date" => "2024-07-21"})

assert %{
"data" => [
%{
"signed_in_by" => "Preferredy Person"
}
]
} = json_response(conn, 200)
end

@tag :authenticated
test "shows official's email address if name is unavailable", %{conn: conn} do
insert(:operator_sign_in,
signed_in_at: DateTime.new!(~D[2024-07-21], ~T[12:00:00], @timezone),
signed_in_by_user:
build(:user, %{
email: "[email protected]"
})
)

conn = get(conn, ~p"/api/signin", %{"line" => "blue", "service_date" => "2024-07-21"})

assert %{
"data" => [
%{
"signed_in_by" => "[email protected]"
}
]
} = json_response(conn, 200)
end

@tag :authenticated
test "sorted by signed_in_at descending (recent first)", %{conn: conn} do
insert(:operator_sign_in,
Expand Down
Loading