pierrejeambrun commented on code in PR #58016:
URL: https://github.com/apache/airflow/pull/58016#discussion_r2549807172
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -63,6 +64,33 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
};
const onError = (_error: unknown) => {
+ // Check if error is a 403 (Forbidden) status
+ if (isHttpError(_error, 403)) {
+ toaster.create({
+ description: translate(
+ "triggerDag.toaster.error.forbidden.description",
+ "You don't have permission to trigger this DAG",
+ ),
+ title: translate("triggerDag.toaster.error.forbidden.title",
"Permission Denied"),
+ type: "error",
+ });
+ } else if (isHttpError(_error, 401)) {
+ toaster.create({
+ description: translate(
+ "triggerDag.toaster.error.unauthorized.description",
+ "Please log in to trigger DAGs",
+ ),
+ title: translate("triggerDag.toaster.error.unauthorized.title",
"Authentication Required"),
+ type: "error",
+ });
Review Comment:
This should almost never happen, user is using the UI, token is
automatically refreshed, so I don't see how we could end up with a 401 response
just for triggering the run. (The whole UI would 401)
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -63,6 +64,33 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
};
const onError = (_error: unknown) => {
+ // Check if error is a 403 (Forbidden) status
+ if (isHttpError(_error, 403)) {
+ toaster.create({
+ description: translate(
+ "triggerDag.toaster.error.forbidden.description",
+ "You don't have permission to trigger this DAG",
+ ),
+ title: translate("triggerDag.toaster.error.forbidden.title",
"Permission Denied"),
+ type: "error",
+ });
+ } else if (isHttpError(_error, 401)) {
+ toaster.create({
+ description: translate(
+ "triggerDag.toaster.error.unauthorized.description",
+ "Please log in to trigger DAGs",
+ ),
+ title: translate("triggerDag.toaster.error.unauthorized.title",
"Authentication Required"),
+ type: "error",
+ });
+ } else {
+ // Generic error handling for other status codes
+ toaster.create({
+ description: translate("triggerDag.toaster.error.generic.description",
"Failed to trigger DAG"),
+ title: translate("triggerDag.toaster.error.generic.title", "Error"),
Review Comment:
You need to add those translation keys to the translation files. Similarly
to `triggerDag.toaster.success.title`.
For instance if you check `en/components.json`
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -63,6 +64,33 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
};
const onError = (_error: unknown) => {
+ // Check if error is a 403 (Forbidden) status
+ if (isHttpError(_error, 403)) {
+ toaster.create({
+ description: translate(
Review Comment:
Maybe an easier way to handle all cases instead of doing some manual mapping
is to just display the `description: error.message`.
Similarly to how we do it in other places, for instance
`usePatchTaskInstance`. This way the backend error is shown to the user.
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -94,5 +122,11 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
});
};
- return { error, isPending, triggerDagRun };
+ return {
+ error,
+ isForbidden: isHttpError(error, 403),
Review Comment:
Doesn't seem used.
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -94,5 +122,11 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
});
};
- return { error, isPending, triggerDagRun };
+ return {
+ error,
+ isForbidden: isHttpError(error, 403),
+ isPending,
+ isUnauthorized: isHttpError(error, 401),
Review Comment:
Same
--
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]
