ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 30 forks source link

Handle save errors #415

Closed will-moore closed 3 years ago

will-moore commented 3 years ago

Fixes #365 and fixes #352

This handles errors when saving figures.

EDIT - we now store the current figure JSON in local storage and direct the user to login page which redirects to /figure/recover/ page which will load the recovered figure.

Screenshot 2021-01-10 at 23 42 32

To test:

jburel commented 3 years ago

I ran the workflow described in the PR

will-moore commented 3 years ago

@jburel I used a different strategy (auto-saving to localstorage and then recovering it after login) to make it easier for users and safer. See edited description above.

jburel commented 3 years ago

Also checking the connection should happen when for example the user clicks on export PDF otherwise the wheel will spin forever and the user won't know that he/she is disconnected.

will-moore commented 3 years ago

@jburel Sorry - that's a bug from another PR. Should be fixed by https://github.com/ome/omero-figure/pull/416/commits/2c3d787a9486960c53540c7fb88e0451746825b6

jburel commented 3 years ago

This time, recovering from local storage works. Ideally the Recover Figure button should be greyed out when there is no figure to recover.

jburel commented 3 years ago

Maybe I am missing something When I recover the image from the Local Storage, the message indicates that the figure has been removed from the local storage. After that I will assume the Local Storage will be empty (I only have one figure in) but it is not the case, I can still recover the figure again and again.

Also the message when clicking on save could be improve Title could be save: Error 403 and remove Error 403 in the core of the dialog. It does not give much since I cannot see the error.

Screenshot 2021-01-19 at 15 44 53
will-moore commented 3 years ago

When you recover from local storage, it doesn't say that the figure has been removed from local storage:

Screenshot 2021-01-19 at 16 51 40

I don't automatically delete from local storage because the user may still not have saved it. It's better that the user explicitly deletes it when they want to. I don't think it's a problem to allow the user to recover the figure more than once.

I'll look at improving the error...

jburel commented 3 years ago

@will-moore I read "removed" instead of "recovered" leading to the comment. In that case, that's fine. Only one dialog to fix in that case

will-moore commented 3 years ago

Dialog updated:

Screenshot 2021-01-20 at 23 06 17
jburel commented 3 years ago

typo noticed when clicking on local storage after clear it.

Screenshot 2021-01-21 at 17 33 20

I assume you mean currently

jburel commented 3 years ago

Previous comment Will you implement the same for Save to PDF? it spins if disconnected

will-moore commented 3 years ago

@jburel Thanks - typo fixed. I'm not sure what to do for Exporting figures. I'd want to distinguish between connection errors and other script export errors. I guess I would handle connection failures in a similar way, but maybe just pop-up a login dialog. I was a bit more paranoid for handling Save errors since the app gets into a more messy state (needs a refresh) and there is the concern about losing data. Export failure is less of a worry since you can probably re-login and the Export or Save as normal. But I'd prefer not to do it in this PR. It's less urgent (not worried about data loss) and this PR is already quite long, and conflicting with #416. So if this is good to merge, I can fix that conflict etc. I'll created an issue for handling figure Export failures: https://github.com/ome/omero-figure/issues/418

jburel commented 3 years ago

Agree that we can do it another PR, it certainly does not seem to put the app in a nasty state,