pierrejeambrun commented on code in PR #68833:
URL: https://github.com/apache/airflow/pull/68833#discussion_r3493255507
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/event_logs.py:
##########
@@ -54,6 +56,65 @@
event_logs_router = AirflowRouter(tags=["Event Log"], prefix="/eventLogs")
+def _get_user_display_name(user) -> str:
+ """Return the most readable identity label available for audit logs."""
+ first_name = (getattr(user, "first_name", None) or "").strip()
+ last_name = (getattr(user, "last_name", None) or "").strip()
+ email = (getattr(user, "email", None) or "").strip()
+ username = (getattr(user, "username", None) or "").strip()
+
+ if first_name and last_name and last_name != "-":
+ return f"{first_name} {last_name}"
+ if email:
+ return email
+ return username
+
+
+def _get_owner_display_names_by_owner(owners: Iterable[str | None], session:
Session) -> dict[str, str]:
+ owner_values = {owner for owner in owners if owner}
+ if not owner_values:
+ return {}
+
+ try:
+ from airflow.providers.fab.auth_manager.models import User
+ except ImportError:
+ return {}
+
+ try:
+ users = session.scalars(
+ select(User).where(or_(User.username.in_(owner_values),
User.email.in_(owner_values)))
+ )
+ except SQLAlchemyError:
+ session.rollback()
+ return {}
+
+ owner_display_names_by_owner: dict[str, str] = {}
+ for user in users:
+ display_name = _get_user_display_name(user)
+ if user.username in owner_values:
+ owner_display_names_by_owner[user.username] = display_name
+ if user.email in owner_values:
+ owner_display_names_by_owner[user.email] = display_name
+
+ return owner_display_names_by_owner
+
+
+def _resolve_owner_display_name(event_log: Log, owner_display_names_by_owner:
dict[str, str]) -> str | None:
+ if event_log.owner_display_name:
+ return event_log.owner_display_name
+ if not event_log.owner:
+ return None
+ return owner_display_names_by_owner.get(event_log.owner) or event_log.owner
+
+
+def _event_log_response(event_log: Log, owner_display_names_by_owner:
dict[str, str]) -> EventLogResponse:
+ return EventLogResponse.model_validate(event_log).model_copy(
+ update={
+ "owner_display_name": _resolve_owner_display_name(event_log,
owner_display_names_by_owner),
+ }
+ )
+
Review Comment:
Set the attribute before model validation
##########
airflow-core/src/airflow/ui/src/pages/Events/Events.tsx:
##########
@@ -71,6 +71,15 @@ const eventsColumn = (
skeletonWidth: 10,
},
},
+ {
+ accessorKey: "owner_display_name",
+ cell: ({ row: { original } }) => original.owner_display_name?.replace(/
-$/u, ""),
+ enableSorting: false,
+ header: translate("auditLog.columns.userDisplayName"),
+ meta: {
+ skeletonWidth: 10,
+ },
+ },
Review Comment:
I think we can just replace the `owner name` column with `owner_display_name`
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/event_logs.py:
##########
@@ -54,6 +56,65 @@
event_logs_router = AirflowRouter(tags=["Event Log"], prefix="/eventLogs")
+def _get_user_display_name(user) -> str:
+ """Return the most readable identity label available for audit logs."""
+ first_name = (getattr(user, "first_name", None) or "").strip()
+ last_name = (getattr(user, "last_name", None) or "").strip()
+ email = (getattr(user, "email", None) or "").strip()
+ username = (getattr(user, "username", None) or "").strip()
+
+ if first_name and last_name and last_name != "-":
+ return f"{first_name} {last_name}"
+ if email:
+ return email
+ return username
+
+
+def _get_owner_display_names_by_owner(owners: Iterable[str | None], session:
Session) -> dict[str, str]:
+ owner_values = {owner for owner in owners if owner}
+ if not owner_values:
+ return {}
+
+ try:
+ from airflow.providers.fab.auth_manager.models import User
+ except ImportError:
+ return {}
Review Comment:
? auth_manager in use can be something different.
We shouldn't import auth managers like this from core, it's accessible via a
dep / app state, you can check other routes to achieve that
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/event_logs.py:
##########
@@ -54,6 +56,65 @@
event_logs_router = AirflowRouter(tags=["Event Log"], prefix="/eventLogs")
+def _get_user_display_name(user) -> str:
+ """Return the most readable identity label available for audit logs."""
+ first_name = (getattr(user, "first_name", None) or "").strip()
+ last_name = (getattr(user, "last_name", None) or "").strip()
+ email = (getattr(user, "email", None) or "").strip()
+ username = (getattr(user, "username", None) or "").strip()
+
+ if first_name and last_name and last_name != "-":
+ return f"{first_name} {last_name}"
+ if email:
+ return email
+ return username
+
+
+def _get_owner_display_names_by_owner(owners: Iterable[str | None], session:
Session) -> dict[str, str]:
+ owner_values = {owner for owner in owners if owner}
+ if not owner_values:
+ return {}
+
+ try:
+ from airflow.providers.fab.auth_manager.models import User
+ except ImportError:
+ return {}
+
+ try:
+ users = session.scalars(
+ select(User).where(or_(User.username.in_(owner_values),
User.email.in_(owner_values)))
+ )
+ except SQLAlchemyError:
+ session.rollback()
+ return {}
+
+ owner_display_names_by_owner: dict[str, str] = {}
+ for user in users:
+ display_name = _get_user_display_name(user)
+ if user.username in owner_values:
+ owner_display_names_by_owner[user.username] = display_name
+ if user.email in owner_values:
+ owner_display_names_by_owner[user.email] = display_name
Review Comment:
Use `user.get_name` which is the public interface on the BaseUser. Not all
users from all auth manager have an `email` or `username`
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/event_logs.py:
##########
@@ -54,6 +56,65 @@
event_logs_router = AirflowRouter(tags=["Event Log"], prefix="/eventLogs")
+def _get_user_display_name(user) -> str:
+ """Return the most readable identity label available for audit logs."""
+ first_name = (getattr(user, "first_name", None) or "").strip()
+ last_name = (getattr(user, "last_name", None) or "").strip()
+ email = (getattr(user, "email", None) or "").strip()
+ username = (getattr(user, "username", None) or "").strip()
+
+ if first_name and last_name and last_name != "-":
+ return f"{first_name} {last_name}"
+ if email:
+ return email
+ return username
+
+
+def _get_owner_display_names_by_owner(owners: Iterable[str | None], session:
Session) -> dict[str, str]:
+ owner_values = {owner for owner in owners if owner}
+ if not owner_values:
+ return {}
+
+ try:
+ from airflow.providers.fab.auth_manager.models import User
+ except ImportError:
+ return {}
+
+ try:
+ users = session.scalars(
+ select(User).where(or_(User.username.in_(owner_values),
User.email.in_(owner_values)))
+ )
+ except SQLAlchemyError:
+ session.rollback()
+ return {}
Review Comment:
No manual rollback.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/event_logs.py:
##########
@@ -54,6 +56,65 @@
event_logs_router = AirflowRouter(tags=["Event Log"], prefix="/eventLogs")
+def _get_user_display_name(user) -> str:
Review Comment:
Duplicated. Put this into a service (similar to other services for the API
layer)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
