firefly-84-TimeSeries tool: Add workspace option to download dialog for packaging images #832

lznakano · 2019-07-10T23:13:05Z

See ticket here: https://jira.ipac.caltech.edu/browse/FIREFLY-159
and here: https://jira.ipac.caltech.edu/browse/FIREFLY-84

Add LcDownloadPanel.jsx
Instead of having three DownloadPanels, only one LcDownloadPanel is use for all missions.
Instead of duplicating onNewRawTable, rawTableRequest and onFieldUpdate for each mission, these three functions are put in LcUntils.js.

I tried to use existing DownloadDialog but I have to make quite some modifications. Instead, I added a
LcDownloadPanel. I modified the BackgroundCntrl to save the files in the workspace. Please let me know if there is any better approach.

URL: https://irsawebdev9.ipac.caltech.edu/firefly-Lijun-firefly-84-firefly-159/irsaviewer/ts.html

loitly · 2019-07-11T23:21:43Z

A few things off the top..

Please include URL to ticket.
Ticket is horrible. Nothing in description. I went and read all of the comments and discovered that it's was all dated in 2018.
When this is the case, will you update the ticket with basic information on what this ticket is requesting for and/or steps to test it.
I know the title say a lot. But, I was looking for more and found nothing.

loitly · 2019-07-12T00:16:08Z

Without requirements, I can only assume how it should work. Based on that, here are my comments.

File Location: Local vs Workspace
This only works if it does not go into Background Monitor. This happens when it's a bigger job and takes longer than a few seconds.
When Workspace is selected and it successfully saved into workspace, there's no indication that it happened. I thought it failed silently.
While preparing, it used to offer a send to background option. I don't see that anymore.

Also, a few items I noticed that is not related to this PR changes, but need to address:

The Upload dialog is clipped on the bottom. It was not the case in OPS.
On the Download Dialog, when you click Save, the dialog hides immediately. No indication it's working, and no option to Send to background.
Background Monitor, click on Download Now and the spiny never stopped. It is working in OPS.

loitly · 2019-07-12T00:22:04Z

I did not review the code. I went and look at FinderChart and noticed that it's behaving the same.
I don't think this behavior is acceptable. There is also a lot of things that make downloading really awkward to use. The same can be said about FinderChart as well. I recommend putting more effort into making it work properly.

lznakano · 2019-07-12T17:40:22Z

Thanks you @loitly very much for your review and great input and suggestions.

Without requirements, I can only assume how it should work. Based on that, here are my comments.

File Location: Local vs Workspace
This only works if it does not go into Background Monitor. This happens when it's a bigger job and takes longer than a few seconds.
I was kind confused what is the proper behavior here. I thought that to save to local, it can go through background monitor, to save to workspace, it does not go through background monitor.

When Workspace is selected and it successfully saved into workspace, there's no indication that it happened. I thought it failed silently.
It is true. What kind indicator would you like to see? Is it OK to use a message box?

While preparing, it used to offer a send to background option. I don't see that anymore.

I will take a look about those.

Also, a few items I noticed that is not related to this PR changes, but need to address:

The Upload dialog is clipped on the bottom. It was not the case in OPS.

On the Download Dialog, when you click Save, the dialog hides immediately. No indication it's working, and no option to Send to background.

Background Monitor, click on Download Now and the spiny never stopped. It is working in OPS.
OK, I will spend more effort to address the issues both in Finderchart and TimeSeries.

ejoliet

Looks perfect!

loitly · 2019-07-17T23:23:01Z

@lznakano The PR is reopened, but I did not get a notice to redo my review.
The test build is still the old one. Behavior has not changed.
@ejoliet , you don't care about the issues I raised in regard to backgrounding?

ejoliet · 2019-07-18T01:26:16Z

Wait, i messed up, the comment was not for this PR , sorry! I just start reading. The comment

Looks perfect!

was for #836 ;-)

ejoliet

I've found same issue about the spinning wheel that doesn't stop. Apart from that, when i saved to workspace, is actually not saving it in workspace, but locally.
Are you sure the link of the deployed version is up to date?
Let me know when i can retest. Thanks.
(the other issues about 'send to background' button it's old story that we never get to the point of agreeing with Vandana so we will have to bring it back in discussion sometime in CCB i guess)

lznakano · 2019-07-18T18:24:03Z

I think that I accidentally closed the pull request. I want to keep it open until I am done with the issues Loi raised. Most of issues were in the dev and were not bugs introduced in this PR. I issued two tickets for those issues presented in the current dev. I am working on one of the ticket, FIREFLY-159, in this PR. I will let you know when it is ready to review again. Thanks!

loitly · 2019-07-18T18:48:26Z

@lznakano , you may want to edit your previous commit. I am sure you did not mean to include that comment. It's over 150 lines of repeated error messages.

lznakano · 2019-07-19T23:52:45Z

This pull request included the bug fixes for Firefly-159. Please test it against both tickets.
When the file is to save to the local file system, the "Send to background" option is enabled. When the file is to save to the workspace, the "Send to background" option is disabled. The local job is sent to background monitor automatically if it is a big job. A message box is displayed after the file is saved to the workspace successfully. I think that we need to discuss what is the expected behavior to save files. Currently, the version in ops does not have "Send to background" button, the job is automatically sent to the background. Please review it again! Thanks!

loitly

First, the test url does not work. I can't test it.
Then the code changes, it looks like you've taken other people changes into your PR. This does not look normal. Please double check to ensure you don't override other changes.
Based on the code changes, I've already raised a couple of concerns. Once you have a url I can test, I'll review the UI.

loitly · 2019-08-15T00:09:39Z

src/firefly/java/edu/caltech/ipac/firefly/server/servlets/AnyFileDownload.java

+            } catch (Exception e) {
+                e.printStackTrace();
+            }
+        }


I don't understand this block. You throw an exception, then catch it to print its stacktrace.
Also, if you plan to log the error, use a logger, not standard out.

src/firefly/js/core/background/BackgroundCntlr.js

loitly · 2019-08-15T00:17:09Z

src/firefly/js/core/background/BgMaskPanel.jsx

@@ -28,13 +28,20 @@ export class BgMaskPanel extends SimpleComponent {
            bgStatus && dispatchJobAdd(bgStatus);
        };

+        const button = bgStatus && bgStatus.isLocal && bgStatus.isLocal?


bgStatus.isLocal is repeated twice

src/firefly/js/core/background/BgMaskPanel.jsx

loitly · 2019-08-15T00:23:01Z

src/firefly/js/templates/lightcurve/LcResult.jsx

-       }
-    }
-}
+/*


What happen here? Why github thinks you've changes to the whole file? Are you sure it's not some sort of git related issues? Double check to make sure you don't override previous changes.

loitly · 2019-08-15T00:23:56Z

src/firefly/js/templates/lightcurve/LcUtil.jsx

@@ -3,14 +3,18 @@ import PropTypes from 'prop-types';
 import {get, has, set, isEmpty} from 'lodash';


Same here. It looks like you took other people changes as your own.

No idea what happened. In my local branch, it is fine. It showed the last modification is July 19. I also compared with dev.

loitly · 2019-08-15T00:24:10Z

src/firefly/js/templates/lightcurve/ptf/PTFMissionOptions.js

@@ -1,15 +1,10 @@
 import React, {PureComponent} from 'react';


lznakano · 2019-08-15T22:05:55Z

@loitly @ejoliet
Thank Loi very much for fixing the line separator issue. I compared a few files and found only LcResult is bad. I copied the one Loi corrected to my branch. I also fixed two issues Loi noticed
and a bug I introduced. Can you please test it when you get time? Thanks!
Test URL: https://irsawebdev9.ipac.caltech.edu/firefly-84-Lijun/irsaviewer/ts.html

ejoliet · 2019-08-16T23:40:04Z

I can't test it via the link but i will locally.
(I see this :

Bad Request
Your browser sent a request that this server could not understand.
Reason: You're speaking plain HTTP to an SSL-enabled server port.
Instead use the HTTPS scheme to access this URL, please.

lznakano · 2019-09-03T18:27:59Z

I rebased with dev and rebuilt the
URL: https://irsawebdev9.ipac.caltech.edu/firefly-Lijun-firefly-84-firefly-159/irsaviewer/ts.html.

Please test it using this URL. Please check the ticket Firefly-84 and Firefly-159 for implementation information.

lrebull · 2019-09-03T20:27:47Z

#832
https://jira.ipac.caltech.edu/browse/FIREFLY-84
https://jira.ipac.caltech.edu/browse/FIREFLY-159
https://irsawebdev9.ipac.caltech.edu/firefly-Lijun-firefly-84-firefly-159/irsaviewer/ts.html

Firefox 68.0.2 (Mac OS 10.11.6)

Downloading:

one table to disk via diskette icon - fine
one image to disk via diskette icon - fine
all images to disk via “prepare download” - worked.
*** tick box for “Also send me email with URLs to download” does nothing. (as in, I can’t enter an email address, not just that it doesn’t send email.)
Gave me an option to send it to the background monitor, which worked.
Asked for massive download. Got a chance to put in an email in the background monitor itself. It sent me email. Link in the email works.

Can’t make it automatically get dropped into background monitor (mentioned in ticket).

I note that readme in zip files must be timestamped UT because right now (~1pm) they are timestamped ~8pm tonight. This messed with my head when looking for the most recently modified files, where I expected a zip file to be top of the list of most recently modified files.

I can’t log in to this server’s workspace, so I cannot test any of the saving files to the workspace.
(Instead of a login screen, I get “default backend - 404”)

Loading a WISE light curve. From the input data tab, I can select all data, download. This process takes a while.
From the phase folded data tab, I can select all data, download. This process appears to complete instantaneously, regardless of what download options I select (cutouts, full images), which makes me think it’s not working. Indeed, the zip files contain solely a “readme” that says, “Successfully packaged 0 files: 0 B.”

Ticket FIREFLY-84 says:
*When the "Prepare Download" button is pressed after making selection in the table, a new Dialog is displayed, except having other fields, it should have two radio buttons, one is local and the other is workspace. When the workspace button is selected, the workspace should not be opened. - WORKS FOR DISK BUT CAN’T TEST WORKSPACE
*When the "Save" is pressed in the Download dialog, the "Send to background" button should be displayed. — CAN’T GET IT TO DO THAT.
*The big job should be sent to background monitor regardless the "Send to background" is pressed or not. — CAN’T GET IT TO DO THAT.
*If the job is not send to the background, after the search and fetch finishes, if the saving location is "local", the local file system should be opened, if the saving location is workspace, the workspace dialog window should be opened. - I DON’T KNOW WHAT ‘LOCAL FILE SYSTEM SHOULD BE OPENED’ MEANS. IT SAVES THE FILE AND ASKS ME IF I WANT TO JUST SAVE IT OR OPEN IT WITH AN APPLICATION (WHICH IS WHAT IT ALWAYS DOES). CAN’T TEST WORKSPACE
*If the job is sent to the background, when the "Download Now" button is clicked in the background Monitor, the same procedure as above should be performed, and the check box should be placed after finishing downloading the file. - WORKS

Ticket FIREFLY-159 says:
*On the "Prepare {{Download" Dialog}}, when you click {{Save}}, if it is a big job, it was sent to the monitor directly without showing the option to {{Send to background}}. — I HAVE THE OPTION TO SEND IT TO THE BACKGROUND
*{{In the Background Monitor}}, click on {{Download Now}} and the spiny never stopped. - CAN’T REPRODUCE, MUST BE FIXED
*The Download Dialog can not be hidden after the "prepare download" is pressed. - I DON’T KNOW ABOUT ‘HIDDEN’ BUT I CAN KILL THE WINDOW IN ANY OF A NUMBER OF WAYS DURING THE DOWNLOAD PROCESS.
*When the file in background monitor is downloaded, there is no check placed to indicate it is already downloaded. - CHECK MARK APPEARS ONCE DOWNLOADED BUT SEEMS MAYBE TO VANISH IN AN IRREPRODUCIBLE WAY. IF I COULD REPRODUCE IT, I WOULD, AND I KNOW YOU CAN’T FIX IT IF I CAN’T REPRODUCE IT. SO UNTIL I CAN REPRODUCE IT, I AM JUST NOTING THAT IT SEEMS TO APPEAR AND THEN DISAPPEAR LATER.

The behavior after finding a period is completely broken. I know that this wasn’t the focus of the ticket, but that makes me even more confused as to why it’s so thoroughly broken. Send the light curve to the periodogram, get the power spectrum, select/accept, and when it returns ostensibly with a phase-folded light curve, it’s added bunch of outlier points that were not in the original light curve and cannot be filtered out. Made a movie documenting this. Light curve attached too.

lrebull · 2019-09-03T20:28:28Z

light curve attached but it doesn't like the movie. comments and movie and light curve all sent in email and attached to the two Jira tickets.

ejoliet · 2019-09-03T21:51:10Z

@lrebull @lznakano i don't think is worth chasing other 'weird' or problem you might found because this version is based on an old branch, right?
If what you found in here is also broken in irsadev, then i would raise a ticket against dev.

lznakano · 2019-09-03T22:00:52Z

@ejoliet
I rebased with dev this morning. If something is broken here, it could be broken in the dev.

lrebull · 2019-09-03T22:28:58Z

@ejoliet @lznakano it is indeed broken on dev. made a ticket IRSA-3112

lznakano · 2019-09-03T22:30:44Z

@lrebull @ejoliet
I confirmed that the issue Luisa found existed in the dev. I reproduced it using irsadev.ipac.caltech.edu. FYI

1. Add LcDownloadPanel.jsx 2. Instead of having three DownloadPanels, only one LcDownloadPanel is use for all missions. 3. Instead of duplicating onNewRawTable, rawTableRequest and onFieldUpdate for each mission, these three functions are put in LcUntils.js. 4. Add a notification using the message box 5. Add functions to save to the workspace after the job is sentto backgorund

lznakano requested review from ejoliet and loitly July 10, 2019 23:13

lznakano closed this Jul 12, 2019

lznakano reopened this Jul 12, 2019

lznakano closed this Jul 12, 2019

lznakano reopened this Jul 15, 2019

lznakano force-pushed the firefly-84-TimeSeries branch from 4d05138 to bdff943 Compare July 15, 2019 23:06

ejoliet approved these changes Jul 17, 2019

View reviewed changes

ejoliet self-requested a review July 18, 2019 01:26

ejoliet requested changes Jul 18, 2019

View reviewed changes

lznakano force-pushed the firefly-84-TimeSeries branch from 557f09a to bb321d2 Compare July 19, 2019 23:22

lznakano changed the title ~~firefly-84:TimeSeries tool: Add workspace option to download dialog for packaging images~~ firefly-84/firefly-159:TimeSeries tool: Add workspace option and bug fixes Aug 12, 2019

loitly requested changes Aug 15, 2019

View reviewed changes

lznakano force-pushed the firefly-84-TimeSeries branch 2 times, most recently from 0b2799f to bb5afc0 Compare August 15, 2019 21:46

lznakano self-assigned this Aug 30, 2019

lznakano force-pushed the firefly-84-TimeSeries branch from bb5afc0 to 802aef4 Compare September 3, 2019 18:01

lznakano requested a review from lrebull September 3, 2019 18:28

lznakano changed the title ~~firefly-84/firefly-159:TimeSeries tool: Add workspace option and bug fixes~~ firefly-84-TimeSeries tool: Add workspace option to download dialog for packaging images Sep 19, 2019

lznakano force-pushed the firefly-84-TimeSeries branch from 802aef4 to af61e30 Compare September 19, 2019 22:00

lznakano closed this Oct 3, 2019

lznakano deleted the firefly-84-TimeSeries branch October 3, 2019 17:58

		@@ -3,14 +3,18 @@ import PropTypes from 'prop-types';
		import {get, has, set, isEmpty} from 'lodash';

		@@ -1,15 +1,10 @@
		import React, {PureComponent} from 'react';

firefly-84-TimeSeries tool: Add workspace option to download dialog for packaging images #832

firefly-84-TimeSeries tool: Add workspace option to download dialog for packaging images #832

Uh oh!

Conversation

lznakano commented Jul 10, 2019 • edited by ejoliet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loitly commented Jul 11, 2019

Uh oh!

loitly commented Jul 12, 2019

Uh oh!

loitly commented Jul 12, 2019

Uh oh!

lznakano commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ejoliet left a comment

Choose a reason for hiding this comment

Uh oh!

loitly commented Jul 17, 2019

Uh oh!

ejoliet commented Jul 18, 2019

Uh oh!

ejoliet left a comment

Choose a reason for hiding this comment

Uh oh!

lznakano commented Jul 18, 2019

Uh oh!

loitly commented Jul 18, 2019

Uh oh!

lznakano commented Jul 19, 2019

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

loitly Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loitly Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loitly Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

loitly Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

lznakano Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

loitly Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

lznakano commented Aug 15, 2019

Uh oh!

ejoliet commented Aug 16, 2019

Uh oh!

lznakano commented Sep 3, 2019

Uh oh!

lrebull commented Sep 3, 2019

Uh oh!

lrebull commented Sep 3, 2019

Uh oh!

ejoliet commented Sep 3, 2019

Uh oh!

lznakano commented Sep 3, 2019

Uh oh!

lrebull commented Sep 3, 2019

Uh oh!

lznakano commented Sep 3, 2019

Uh oh!

Uh oh!

lznakano commented Jul 10, 2019 •

edited by ejoliet

Loading

lznakano commented Jul 12, 2019 •

edited

Loading