Feature/cancel export rebased & bug fixes to customizable export UI #519

sashadev-sky · 2020-01-11T11:34:47Z

Fixes #0000 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updates
@mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

=============================================

Combines the structure in FEATURE: Cancel export functionality #494 to address the exporter changes in [WIP] customizable export UI for DistortableCollection #485 that throw errors.
Adds new cancel export functionality implemented in FEATURE: Cancel export functionality #494 by @VladimirMikulic
Changes to [WIP] customizable export UI for DistortableCollection #485
- Removed the resolve option because i'm thinking if someone is using the startExport method in the first place, the resolve will be what is passed to the Promise always? Also they can already customize the updater
- Refactored - added the urls as default class options. Updated export.html
  =============================================

Resolves publiclab#479

Changes: - Resolved ESLint errors(ExportAction.js & ToolbarIconSet.js) - Empty line added in assets/icons/svg/cancel.svg

sashadev-sky · 2020-01-11T11:58:16Z

@jywarren labeled this high priority because #485 was not running for me and errored on the POST request. Also please lmk my changes to startExport don't override anything you were planning on implementing

sashadev-sky · 2020-01-11T12:23:08Z

@jywarren Thought 'http//export.mapknitter.org/', was a typo so updated it to have a : but seeing now it doesn't need it. Was wondering how come?

VladimirMikulic · 2020-01-11T12:25:39Z

@sashadev-sky I've cloned this and tested it locally. It works fine. There are no POST requests errors.

VladimirMikulic · 2020-01-11T12:30:35Z

There is one error though when using HTTPS protocol.

Export in export.html doesn't work over HTTPS.

jywarren · 2020-01-11T13:36:28Z

Hi, thanks! Cc @tech4GT were we able to get https running? Thanks all! Exciting to see this getting there!

…

On Sat, Jan 11, 2020, 7:30 AM Vladimir Mikulic ***@***.***> wrote: @sashadev-sky <https://github.com/sashadev-sky> I've cloned this and tested it locally. It works fine. There are no POST requests errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAAF6JY2CSHTN4MWE6EC25TQ5G3EJA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWA3WA#issuecomment-573312472>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J4M44FQYWIDCAXFGHDQ5G3EJANCNFSM4KFRRUAA> .

tech4GT · 2020-01-11T16:26:03Z

Hi, thanks! Cc @tech4GT were we able to get https running? Thanks all! Exciting to see this getting there!
…
On Sat, Jan 11, 2020, 7:30 AM Vladimir Mikulic @.***> wrote: @sashadev-sky https://github.com/sashadev-sky I've cloned this and tested it locally. It works fine. There are no POST requests errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAAF6JY2CSHTN4MWE6EC25TQ5G3EJA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWA3WA#issuecomment-573312472>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J4M44FQYWIDCAXFGHDQ5G3EJANCNFSM4KFRRUAA .

I think I did put in the cors code! :D

jywarren · 2020-01-11T16:41:35Z

Isn't cors distinct from running ssl to get https access?

…

On Sat, Jan 11, 2020, 11:26 AM Varun Gupta ***@***.***> wrote: Hi, thanks! Cc @tech4GT <https://github.com/tech4GT> were we able to get https running? Thanks all! Exciting to see this getting there! … <#m_4975143771099100894_> On Sat, Jan 11, 2020, 7:30 AM Vladimir Mikulic *@*.***> wrote: @sashadev-sky <https://github.com/sashadev-sky> https://github.com/sashadev-sky I've cloned this and tested it locally. It works fine. There are no POST requests errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519 <#519>?email_source=notifications&email_token=AAAF6JY2CSHTN4MWE6EC25TQ5G3EJA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWA3WA#issuecomment-573312472>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J4M44FQYWIDCAXFGHDQ5G3EJANCNFSM4KFRRUAA . I think I did put in the cors code! :D — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAAF6J7CCS7MOBLFIDTZIQDQ5HXJZA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWFOXQ#issuecomment-573331294>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J5TT7NIB3MBMMKPMITQ5HXJZANCNFSM4KFRRUAA> .

tech4GT · 2020-01-11T16:43:35Z

Isn't cors distinct from running ssl to get https access?
…
On Sat, Jan 11, 2020, 11:26 AM Varun Gupta @.> wrote: Hi, thanks! Cc @tech4GT https://github.com/tech4GT were we able to get https running? Thanks all! Exciting to see this getting there! … <#m_4975143771099100894_> On Sat, Jan 11, 2020, 7:30 AM Vladimir Mikulic @.> wrote: @sashadev-sky https://github.com/sashadev-sky https://github.com/sashadev-sky I've cloned this and tested it locally. It works fine. There are no POST requests errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519 <#519>?email_source=notifications&email_token=AAAF6JY2CSHTN4MWE6EC25TQ5G3EJA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWA3WA#issuecomment-573312472>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J4M44FQYWIDCAXFGHDQ5G3EJANCNFSM4KFRRUAA . I think I did put in the cors code! :D — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAAF6J7CCS7MOBLFIDTZIQDQ5HXJZA5CNFSM4KFRRUAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWFOXQ#issuecomment-573331294>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J5TT7NIB3MBMMKPMITQ5HXJZANCNFSM4KFRRUAA .

Yes you're right! It's not yet working on https! I'll open an issue for this!

VladimirMikulic · 2020-01-11T16:56:16Z

@tech4GT @jywarren is right. SSL is not related to CORS.
This error is a security mechanism because it prevents HTTPS websites to make requests to insecure endpoints(HTTP).
On the other hand, CORS is used to specify who can access the assets on the server, which methods it can use to do so among other things.

VladimirMikulic · 2020-01-11T16:58:50Z

When SSL is enabled all ajax request must also go on https://something.
Simply put you can't make HTTP requests from HTTPS website.

jywarren · 2020-01-11T17:12:16Z

src/edit/DistortableCollection.Edit.js

@@ -320,6 +252,81 @@ L.DistortableCollection.Edit = L.Handler.extend({
    }, this);
    return this;
  },
+
+  startExport: function() {


Hm, are you sure this works? I had to move it into a closure to give it access to the this scope.

Ah, i see, you used bind instead, down on this line

OK!

jywarren · 2020-01-11T17:15:40Z

When SSL is enabled all ajax request must also go on https://something.
Simply put you can't make HTTP requests from HTTPS website.

Yes, and what's happening in some cases is also a complex bug where a 500 error can generate a CORS error, because the 500 error is a different HTTP status code (i think?), and sometimes CORS only allows GET/POST -- so sometimes resolving the underlying 500 error can make CORS errors disappear too. But they could also be legit independent CORS errors. Gah! 😅

jywarren · 2020-01-11T17:18:34Z

NOTE: RESOLVED (see below)

~~Noting that using the external exporter will ALWAYS cause a 500 error when run locally, because your images are not accessible to the export service (which will try to fetch them).~~

@jywarren labeled this high priority because #485 was not running for me and errored on the POST request. Also please lmk my changes to startExport don't override anything you were planning on implementing

~~Were you getting this error running locally or from gh-pages?~~

~~You can also get this working locally by using remote images like this one:~~

~~https://publiclab.github.io/Leaflet.DistortableImage/examples/example.jpg~~

~~Maybe we should do that for all example images, because that'll allow us to test exports locally?~~

jywarren · 2020-01-11T17:20:03Z

@jywarren Thought 'http//export.mapknitter.org/', was a typo so updated it to have a : but seeing now it doesn't need it. Was wondering how come?

I think that was actually a needed fix, but in the example I made in #485 I overrode the default so the example isn't affected by my original typo 😅

jywarren · 2020-01-11T17:20:31Z

I'm going to try changing the example images to use remote ones and see if things work locally then.

jywarren · 2020-01-11T17:22:44Z

Ah, i'd already thought of that, and they are already remote --

https://github.com/sashadev-sky/Leaflet.DistortableImage/blob/1a67e80d720c550a5049acad48622496eef9be6f/examples/export.html#L73

OK, what I did was I changed https:// to // so that we are less likely to trigger CORS, but we'll still have to test this only from an http origin.

jywarren · 2020-01-11T17:27:41Z

😄 🎉 Tested this and it works!

The redirect to the status.json output file is a little weird, but it's cool. Maybe we should build more/better UI around this but I'm already doing that in publiclab/mapknitter#1192, and we can pull out the key JS parts of that and develop an export handler/UI mini repository from that code once it's running more smoothly.

jywarren · 2020-01-11T17:30:13Z

@tech4GT this may not work in gh-pages because we're on https there, but I'll turn of the https-forcing on this repo to see if we can run it without?

VladimirMikulic · 2020-01-11T17:31:41Z

@jywarren I can confirm that this works on both HTTP and HTTPS.

jywarren · 2020-01-11T17:32:34Z

Indeed it doesn't force, so we can try it at http://publiclab.github.io/Leaflet.DistortableImage/examples/export.html -- note NOT HTTPS. 🎉

It's republishing now... also i published v0.11.0 to NPM!

jywarren · 2020-01-11T17:35:30Z

YES! IT WORKS!!!

Very nice folks!!!

we still need HTTPS on the exporter (https://github.com/publiclab/image-sequencer-app) for this to work on Mapknitter, which is on HTTPS. But this is really awesome.

Note that we're overriding the images in this example, so it doesn't matter what you do to the demo images, actually.

jywarren · 2020-01-11T17:36:42Z

This is really cool. @publiclab/is-reviewers this is actually image sequencer running in a headless Node.js kubernetes cluster in Google Cloud, generating full-res MapKnitter exports! 💥

VladimirMikulic · 2020-01-11T17:37:24Z

Well done everyone.

VladimirMikulic and others added 9 commits December 30, 2019 16:40

ADD: Cancel SVG Icon

43fc746

ADD: Cancel export functionality

bed230a

Resolves publiclab#479

FIX: Minor issues

5ac5594

Changes: - Resolved ESLint errors(ExportAction.js & ToolbarIconSet.js) - Empty line added in assets/icons/svg/cancel.svg

REFACTOR: Cancel export to avoid "self" variable

00e2baf

Merge branch 'main' into feature/cancel-export

134669f

Fix exporter and combine with cancel-export functionality

6210b3d

Clean

1ecf0bd

Update export.html

7ae77e2

Clean linting

b4dce67

sashadev-sky added high-priority needs-review labels Jan 11, 2020

sashadev-sky added 2 commits January 11, 2020 06:38

Bump version in package.lock to 0.11.0

12a2efb

Remove console.log outputting data in unuseful format

65be1cc

sashadev-sky requested a review from jywarren January 11, 2020 11:43

sashadev-sky mentioned this pull request Jan 11, 2020

FEATURE: Cancel export functionality #494

Closed

sashadev-sky removed the high-priority label Jan 11, 2020

sashadev-sky added the high-priority label Jan 11, 2020

jywarren mentioned this pull request Jan 11, 2020

Cloud Export integration, saving & display publiclab/mapknitter#1192

Merged

jywarren approved these changes Jan 11, 2020

View reviewed changes

Update export.html

1a67e80

jywarren merged commit 7d6d743 into publiclab:main Jan 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/cancel export rebased & bug fixes to customizable export UI #519

Feature/cancel export rebased & bug fixes to customizable export UI #519

sashadev-sky commented Jan 11, 2020 •

edited

Loading

sashadev-sky commented Jan 11, 2020 •

edited

Loading

sashadev-sky commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020 •

edited

Loading

jywarren commented Jan 11, 2020 via email

tech4GT commented Jan 11, 2020

jywarren commented Jan 11, 2020 via email

tech4GT commented Jan 11, 2020 •

edited

Loading

VladimirMikulic commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020 •

edited

Loading

jywarren Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020 •

edited

Loading

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

jywarren commented Jan 11, 2020 •

edited

Loading

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

Feature/cancel export rebased & bug fixes to customizable export UI #519

Feature/cancel export rebased & bug fixes to customizable export UI #519

Conversation

sashadev-sky commented Jan 11, 2020 • edited Loading

sashadev-sky commented Jan 11, 2020 • edited Loading

sashadev-sky commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020 • edited Loading

jywarren commented Jan 11, 2020 via email

tech4GT commented Jan 11, 2020

jywarren commented Jan 11, 2020 via email

tech4GT commented Jan 11, 2020 • edited Loading

VladimirMikulic commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020 • edited Loading

jywarren Jan 11, 2020

Choose a reason for hiding this comment

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020 • edited Loading

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

jywarren commented Jan 11, 2020 • edited Loading

jywarren commented Jan 11, 2020

jywarren commented Jan 11, 2020

VladimirMikulic commented Jan 11, 2020

sashadev-sky commented Jan 11, 2020 •

edited

Loading

sashadev-sky commented Jan 11, 2020 •

edited

Loading

VladimirMikulic commented Jan 11, 2020 •

edited

Loading

tech4GT commented Jan 11, 2020 •

edited

Loading

VladimirMikulic commented Jan 11, 2020 •

edited

Loading

jywarren commented Jan 11, 2020 •

edited

Loading

jywarren commented Jan 11, 2020 •

edited

Loading