Skip to content

FIREFLY-1748: Cleanup and fix failing tests #1809

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

Merged
merged 1 commit into from
Jul 22, 2025

Conversation

kpuriIpac
Copy link
Contributor

@kpuriIpac kpuriIpac commented Jul 15, 2025

Ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-1748
IFE PR: https://github.com/IPAC-SW/irsa-ife/pull/426

  • fix failing unzip test in download script, re-enabled the test
  • Per an older PR suggestion (FIREFLY-1704: Download Script Enhancement #1745), change usage of BaseFileName to Title (since we merged these fields in the UI), and use Title as the file name
    • this change has been made for the IFE apps as well, refer to the IFE PR above for the same

Testing

  • Spherex, Euclid, Wise, SHA, ZTF, Sofia, Finderchart, DCE
  • For all apps, just do any (only one) search, try and select a few resulting rows and download, ensure the downloaded file name matches the name in the Title field of the download dialog

@kpuriIpac kpuriIpac added this to the 2025.4 milestone Jul 15, 2025
@kpuriIpac kpuriIpac requested a review from loitly July 15, 2025 23:32
@kpuriIpac kpuriIpac self-assigned this Jul 15, 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.

  • If the title is set to abc.zip, the downloaded file ends up as abc.zip.zip. This might be acceptable, but just noting it in case it's unintended.

Yes, this is unintended. I'll see if it's easy enough to fix this.

@kpuriIpac
Copy link
Contributor Author

I thought the goal is to make the download Title matches the Job History Title. I didn't expect it would require changing the actual parameter name and touching so many files. That said, if this change is necessary, it is fine.

Yes so the reason for this is that we used to have two separate fields on the UI, a title and a file name (corresponding to Title and BaseFileName). And on the backend we have 2 functions getTitle and getBaseFileName (in DownloadRequest.java), now both of these return the same thing. So I could just get rid of getBaseFileName and replace all its usage with getTitle, but the only reason I left it as is is because in our logic it makes sense that when you do a getBaseFileName, what you get back (title) is to be treated as a file name.

@kpuriIpac
Copy link
Contributor Author

  • How is the download script generated? I no longer see an option. I only seem to receive it when I request email notification. Is that the expected behavior?

wait, could you clarify this part @loitly? Are you not able to download and then run the script? (if you do curl or wget)

@loitly
Copy link
Contributor

loitly commented Jul 22, 2025

  • How is the download script generated? I no longer see an option. I only seem to receive it when I request email notification. Is that the expected behavior?

wait, could you clarify this part @loitly? Are you not able to download and then run the script? (if you do curl or wget)

In WISE, when I click Download, there's no option to get a script, It just packages and downloads the zip file directly.
If I go to the Job Monitor and click Download, it also downloads the zip file directly.

The only way to get a script is by enabling email notification. The email includes a link to download the script, which can then be used to download the zip file.

This is the observed behavior. If all of this is intended, then it's fine.

@kpuriIpac
Copy link
Contributor Author

In WISE, when I click Download, there's no option to get a script, It just packages and downloads the zip file directly. If I go to the Job Monitor and click Download, it also downloads the zip file directly.

The only way to get a script is by enabling email notification. The email includes a link to download the script, which can then be used to download the zip file.

This is the observed behavior. If all of this is intended, then it's fine.

Oh right, yes that's intended. Wise is one of the apps where we don't do download script by default (eventually we may, I think @robyww prefers that as well). But yes as of now it's the intended functionality.

@kpuriIpac
Copy link
Contributor Author

  • If the title is set to abc.zip, the downloaded file ends up as abc.zip.zip. This might be acceptable, but just noting it in case it's unintended.

Yes, this is unintended. I'll see if it's easy enough to fix this.

@loitly I'm merging for now, as this is technically easy to do but I would like to discuss logic at Thursday's meeting. Because we could normally allow periods in filenames, so we can discuss what the edge cases could be.

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1748-cleanup-fixes branch from 7af3beb to 7489393 Compare July 22, 2025 22:57
@kpuriIpac kpuriIpac merged commit ff2bfb2 into dev Jul 22, 2025
@kpuriIpac kpuriIpac deleted the FIREFLY-1748-cleanup-fixes branch July 22, 2025 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants