Skip to content

FIREFLY-1695: Display error message when upload table can't be read#1756

Merged
jaladh-singhal merged 1 commit intodevfrom
FIREFLY-1695-upload-error-msg
May 6, 2025
Merged

FIREFLY-1695: Display error message when upload table can't be read#1756
jaladh-singhal merged 1 commit intodevfrom
FIREFLY-1695-upload-error-msg

Conversation

@jaladh-singhal
Copy link
Member

Fixes FIREFLY-1695

  • onSubmit of UploadTableChooser (used in Multi-object fields) now uses determineValidity like FileUploadPanel's onSubmit, that shows an error popup if invalid
  • FileUploadPanel was maintaining a message state besides report but wasn't using it in generating invalidation popup, now made it do so by registering in request.additionalParams and passing it to determineValidity.
  • Made message state to read FileAnalysis.ErrorResponse description from server-side for showing more specific error messages.

Testing

https://fireflydev.ipac.caltech.edu/firefly-1695-upload-error-msg/firefly/

Go to "Upload" tab -> Upload the bad table file in ticket -> click "load" button, the error popup should show VOTable parsing error instead of "Could not recognize the file type" error which is misleading.

Go to TAP -> Spatial -> Multi-object -> Upload the bad table file in ticket -> click "load" button, it should show an error popup with same error message as Upload tab.

@jaladh-singhal jaladh-singhal added this to the 2025.3 milestone May 5, 2025
@jaladh-singhal jaladh-singhal requested review from loitly and robyww May 5, 2025 19:55
@jaladh-singhal jaladh-singhal self-assigned this May 5, 2025
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

It’s working as described. However, if possible, it would be better to display the user’s original input instead of the server-side filename/path, which is meaningless to the user.

A related issue, possibly outside the scope of this PR, is with the IRSA Catalog’s Multi-Object search. If a bad file is uploaded, the search still runs but ends up stuck in a loading state.
(IRSA Catalog → Search Method = Multi-Object)

@jaladh-singhal
Copy link
Member Author

jaladh-singhal commented May 5, 2025

However, if possible, it would be better to display the user’s original input instead of the server-side filename/path, which is meaningless to the user.

@loitly yes I had the same thought and also found the relevant code, but wasn't sure what to replace location here with?

throw new IOException("Unable to parse VOTABLE from "+ location + "\n" +
e.getMessage(), e);

OR is there a better way to extract only relevant information from server-side error responses at client-side?

@jaladh-singhal
Copy link
Member Author

jaladh-singhal commented May 5, 2025

A related issue, possibly outside the scope of this PR, is with the IRSA Catalog’s Multi-Object search. If a bad file is uploaded, the search still runs but ends up stuck in a loading state.
(IRSA Catalog → Search Method = Multi-Object)

Good catch, I didn't notice it. I just looked it up, this (and all ife apps) are using an old UploadOptionsDialog instead of newer UploadTableChooser that's being used everywhere else in Firefly. I'm not sure what was the reason of leaving it when @kpuriIpac migrated all upload components. @robyww do you remember? (if we decide to use UploadTableChooser, that migration can be done in a separate ticket)

@robyww
Copy link
Contributor

robyww commented May 6, 2025

I'm not sure what was the reason of leaving it when @kpuriIpac migrated all upload components. @robyww do you remember?

the migration is not done. The image panel needs to be migrated as well.

@jaladh-singhal jaladh-singhal requested a review from kpuriIpac May 6, 2025 01:47
@loitly
Copy link
Contributor

loitly commented May 6, 2025

@loitly yes I had the same thought and also found the relevant code, but wasn't sure what to replace location here with?
OR is there a better way to extract only relevant information from server-side error responses at client-side?

I would change server-side code to only return the error message and allow the caller(client) to add context if needed. So, in your case, the message would look like....

Error in File Retrieve: Unable to parse firefly_1695_bad_table.vot as a VOTABLE.
Reason: The element type "DESCRIPTION" must be terminated by the matching end-tag "".

You can add 'location' to the error log line above for debugging purposes.

LOG.error(e, "unable to parse " + location ");
throw new DataAccessException(e.getMessage(), e);

Copy link
Contributor

@kpuriIpac kpuriIpac left a comment

Choose a reason for hiding this comment

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

Looks good, works for both uploads when you try to load the bad file. Agree with @loitly's comment to simplify the error message by taking out location.

@kpuriIpac
Copy link
Contributor

A related issue, possibly outside the scope of this PR, is with the IRSA Catalog’s Multi-Object search. If a bad file is uploaded, the search still runs but ends up stuck in a loading state.
(IRSA Catalog → Search Method = Multi-Object)

Good catch, I didn't notice it. I just looked it up, this (and all ife apps) are using an old UploadOptionsDialog instead of newer UploadTableChooser that's being used everywhere else in Firefly. I'm not sure what was the reason of leaving it when @kpuriIpac migrated all upload components. @robyww do you remember? (if we decide to use UploadTableChooser, that migration can be done in a separate ticket)

Yes, I can work on this in a new ticket if we want to use UpoadTableChooser here as well @robyww. I may have missed this in the initial migration.

@jaladh-singhal
Copy link
Member Author

jaladh-singhal commented May 6, 2025

I would change server-side code to only return the error message and allow the caller(client) to add context if needed.

@loitly thanks, I logged the location and I'm only returning exception's message from server-side. The error on client-side looks much cleaner now. Rebuilt the build

FIREFLY-1695: use determineValidity in UploadTableChooser to show error

Fallback to a generic error popup instead of just returning without any feedback

FIREFLY-1695: Use error response returned by server in determineValidity

FIREFLY-1695: Make server-side file analysis return only the error message & log the rest
@jaladh-singhal jaladh-singhal force-pushed the FIREFLY-1695-upload-error-msg branch from f3ccf76 to d46dc11 Compare May 6, 2025 22:03
@jaladh-singhal jaladh-singhal merged commit a262ce4 into dev May 6, 2025
@jaladh-singhal jaladh-singhal deleted the FIREFLY-1695-upload-error-msg branch May 6, 2025 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants