Connect LDI exports downstream #782

sashadev-sky · 2019-07-02T16:30:10Z

Fixes #756 (<=== 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 rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/mapknitter-reviewers for help, in a comment below

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!

codeclimate · 2019-07-02T16:30:54Z

Code Climate has analyzed commit e673eaf7 and detected 0 issues on this pull request.

View more on Code Climate.

codecov · 2019-07-02T17:54:48Z

Codecov Report

Merging #782 into main will increase coverage by 1.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
+ Coverage   71.41%   72.43%   +1.01%     
==========================================
  Files          35       35              
  Lines        1333     1335       +2     
==========================================
+ Hits          952      967      +15     
+ Misses        381      368      -13

Impacted Files	Coverage Δ
app/controllers/front_ui_controller.rb	`77.77% <100%> (+27.77%)`	⬆️
app/controllers/export_controller.rb	`87.27% <100%> (+0.73%)`	⬆️
app/models/map.rb	`93.1% <0%> (+0.68%)`	⬆️
app/controllers/application_controller.rb	`87.09% <0%> (+3.22%)`	⬆️
app/helpers/front_ui_helper.rb	`100% <0%> (+42.85%)`	⬆️

codeclimate · 2019-07-02T18:34:09Z

app/assets/javascripts/mapknitter/Map.js

+      actions: [Exports]
+    }).addTo(map);
+
+    /** TODO: make an addLayers func */


jywarren · 2019-07-02T19:09:51Z

https://github.com/publiclab/plots2/blob/65096e88b43baa9749df821976ccfe00ee362e66/app/assets/javascripts/setup.js#L2

sashadev-sky · 2019-07-02T22:28:18Z

@jywarren this is working now! The code still needs a lot of cleaning but whats happening here is we are passing a custom action to the collection, which is created in the customExportAction function.

There, we overrwrite the handler and updater and pass them in the bottom:

 group.startExport({ handleStatusUrl: addUrlToModel, updater: updateUI });

the handler

makes the bottom POST request first to save the url in the Export model. I left some comments but from my understanding we will be getting rid of all the columns in the Export model, but keep one just for updating the url? I used export_url column
makes the 2nd ajax call to the updater. The updater doesn't do anything other than console.log yet. I still am unsure how to get a complete message. mine is stuck on warping 1/1

      var addUrlToModel = function(data) {
          var statusUrl = "//export.mapknitter.org" + data;
          console.log("statusUrl: " + statusUrl);

            // repeatedly fetch the status.json
          var updateInterval = function () {
            setInterval(function intervalUpdater() {
              $.ajax(statusUrl, {
                type: "GET",
                crossDomain: true
              }).done(function (data) {
                updateUI(data);
              });
            }, 3000);
          }

          window.updateInterval = updateInterval;
          /**
           * TODO: update API to say if you pass in a custom `handleStatusUrl` you must also build your own updater
           * and also create your own a frequency
           * or fix this part
           */
          $.ajax({
            url: "/export",
            type: 'POST',
            data: { status_url: statusUrl }
          }).done(function (data) {
            console.log('success!!' + data);
            updateInterval();
          });
        }

Need advice on how to proceed based on the above points!

jywarren · 2019-07-02T22:39:56Z

OK, so we need to supply an updater() method that'll get past the cache problem; we're just seeing the same browser-cached response; it's not actually stuck -- try hitting the true URL and you'll see:

$.ajax("http://mapknitter-exports-warps.storage.googleapis.com/1562106910/status.json", {
  type: "GET",
  crossDomain: true
}).done(function(data) {
  console.log(data);
});

So, you can add a timestamp to the end of the URL and it'll bust the cache now:

$.ajax("http://export.mapknitter.org/id/1560202769/status.json?1", {
  type: "GET",
  crossDomain: true
}).done(function(data) {
  console.log(data);
});

That should work!

jywarren · 2019-07-02T22:40:04Z

You can test it here: http://jywarren.github.io/Leaflet.DistortableImage/examples/select

sashadev-sky · 2019-07-02T22:40:22Z

note that if we overrwrote every action we could probably get around the svg icon issue quickly if that becomes necessary

jywarren · 2019-07-02T22:41:32Z

OK if you can't solve the SVG issue otherwise we can do that as a quick solution and solve later...

Note - i'm going to fix the cache-busting upstream in the default in LDI too, i'll open a PR and link here. But it shouldn't matter for this PR if you're overriding the updater anyways!

sashadev-sky · 2019-07-02T22:42:19Z

@jywarren right ok ill try that! Sounds good!

jywarren · 2019-07-02T22:44:33Z

See publiclab/Leaflet.DistortableImage#318

jywarren · 2019-07-02T22:53:54Z

OK just note I put the cache buster in the wrong place in my PR (i'm updating now) -- i put it in _defaultHandleStatusUrl but that's not right. It needs to be in _fetchStatusUrl and i just fixed that in my PR.

jywarren · 2019-07-02T22:54:39Z

I think the Date.now() has to go inside the interval function or it won't get a fresh timestamp each time, you know? Silly mistake on my part!

jywarren · 2019-07-02T22:56:24Z

And will this PR have the spinner appear? Thanks @sashadev-sky we are really almost there! Great work!

jywarren · 2019-07-02T22:57:05Z

http://jywarren.github.io/Leaflet.DistortableImage/examples/select is now working with the code from publiclab/Leaflet.DistortableImage#318 !

sashadev-sky · 2019-07-02T22:58:12Z

Yes I can add that on next!

jywarren · 2019-07-02T22:59:11Z

OK we won't be able to merge this unless Travis fixes its outage, but let's get everything ready and we can likely first thing tomorrow morning!

sashadev-sky · 2019-07-02T23:02:33Z

@jywarren ok so should I just overrwrite every action to fix the icons? I can do some more research on it but I can't say if i'll find the right answer / how long will take if it is in fact a leaflet-toolbar issue.

The good thing is we are almost at rails 5 where we can integrate things like webpack for our JS

sashadev-sky · 2019-07-02T23:14:31Z

@jywarren it is still not working for me from MK after 8 minutes

jywarren · 2019-07-02T23:14:50Z

Yeah... i guess so. Let's mark the lines you're adding with the short-term fix and link to an issue in LDI from in the in-code comments so we're sure to fix this later. But I think that's the best path for now. Thank you!

jywarren · 2019-07-02T23:15:15Z

And let's link from the new issue on the SVG stuff /back/ to the relevant lines of code to be fixed downstream as well?

sashadev-sky · 2019-07-02T23:46:45Z

the cache bust was not working was not referring to the icons in my last comment! but sounds good for the icons

sashadev-sky · 2019-07-03T04:38:50Z

@jywarren I rewrote the icons. They use inline svg, which could also be used upstream in LDI. the syntax isn't worse in my opinion but I read that it bloats pages and is difficult to cache. I think maybe to just leave it and I should try again with the pipeline in Rails 5? I don't think its a leaflet-toolbar issue because I copied the _createIcon method there into L.EditActions and it still threw the error.

I couldn't get the exporting to work with the cache busting so there's no UI yet but we already use leaflet-spin in this library so that will be quick to add in!

get collection group API working custom export action working clean up more cleanup

jywarren · 2019-07-03T15:55:33Z

Hmm, i'm seeing:

Uncaught TypeError: this.imgActionArray is not a function
    at Object.<anonymous> (VM525 Map.self-c76b499773e2ed70a649cbce2df6162c1988da7e73aad1b2376e307b41ae80a3.js:106)

For this line:

          var img = L.distortableImageOverlay(
            warpable.srcmedium,
            {
              keymapper: false,
              actions: this.imgActionArray(),
              corners: corners,
              mode: 'lock'
            }).addTo(map);

app/assets/javascripts/mapknitter/Map.js

jywarren · 2019-07-03T16:04:44Z

Solved the first issue. Now seeing:

Uncaught TypeError: img.editing._toggleRotateDistort is not a function -- we renamed this?
can't select an image... looking into it...

jywarren · 2019-07-03T20:46:31Z

OK, this works now! The multiple export button itself, with no other manual scripting, generates this:

https://mapknitter-exports-warps.storage.googleapis.com/1562186552/1562186552.jpg

Hooray! I'm adding an "alert" when it completes, and merging this asap!

jywarren · 2019-07-03T20:59:49Z

Awesome:

Alert is still not working. Looking...

sashadev-sky · 2019-07-03T21:01:48Z

app/assets/javascripts/mapknitter/Map.js

              keymapper: false,
+              actions: mapknitter.imgActionArray(),


this line is being called on mapknitter and the other time its called its called on this. but I believe they both work so it should just be called on this? Not sure because I was never getting the error you pointed out:

Uncaught TypeError: this.imgActionArray is not a function at Object.<anonymous> (VM525 Map.self-c76b499773e2ed70a649cbce2df6162c1988da7e73aad1b2376e307b41ae80a3.js:106)

Suggested change

actions: mapknitter.imgActionArray(),

actions: this.imgActionArray(),

For images added at runtime by the user, this works, but for images that were loaded on the page already at load time, this was not being preserved through the nested constructors. So I assigned this to a local variable mapknitter with the correct scope, to get past the errors. In general, this usage is a bit brittle since it can so easily be overridden, so fixing it to a named locally scoped variable is a pretty good practice.

jywarren · 2019-07-03T21:09:23Z

OK got it -- the status JSON is passed in the data param as a string. We need to JSON.parse(data).

sashadev-sky · 2019-07-03T21:09:52Z

@jywarren for me, multiple image select is no longer possible with the pulled in changes. The images highlight yellow but only one image can be dragged at a time. Looking into it

jywarren · 2019-07-03T21:11:55Z

Hmm, i'm able to drag these two images on unstable -- can you?

http://mapknitter-unstable.laboratoriopublico.org/maps/mestia

jywarren · 2019-07-03T21:12:30Z

Oh sorry it's building with the JSON.parse fix. Also check out publiclab/Leaflet.DistortableImage#321 as this will make testing much easier!

jywarren · 2019-07-03T21:15:41Z

I think that although without publiclab/Leaflet.DistortableImage#321 it's not super easy to use, this is worth merging and publishing to try out on production. It's a bit hidden of a feature right now, which for early release is actually a good thing! We can try exporting a bunch of maps, esp. on stable.

Actually I need to check with icarito about SSL on the exporter container anyways!

sashadev-sky · 2019-07-03T21:31:31Z

@jywarren ok I don't know why it wont allow me to multiple select but it had some conflicts when I pulled it in so I could've missed something there. If its working in unstable for you I'd say we can go for a merge!

jywarren · 2019-07-03T21:33:07Z

I keep making silly mistakes on handling the response, but as soon as I get it to put up an alert with the completed JPG URL I'll merge it. Thanks!!!

jywarren · 2019-07-03T21:45:09Z

Awesome! It worked! Merging this as soon as it passes. Next steps would be:

spinner
ability to select without unlocking

…mk-exports

jywarren · 2019-07-03T21:49:37Z

OK, fixed a test. I think we'll be good now!

jywarren · 2019-07-03T21:50:33Z

@divyabaid16 note here that Sasha has started storing export_url with the location of a remote status.json file, that can be used to display the export status!

sashadev-sky · 2019-07-03T22:08:58Z

@jywarren ok does the spinner need to be done tonight?

jywarren · 2019-07-03T22:12:55Z

We win!!! No, I think we're good to publish to the live site now. Spinner can be any time you like -- would it be here or upstream in LDI?

jywarren · 2019-07-03T22:13:35Z

Thanks for sticking with this, @sashadev-sky ! The dots are connected!

sashadev-sky · 2019-07-03T22:46:51Z

@jywarren Awesome! So what I want is to make an animated svg spinner icon that replaces the export icon in LDI while it's exporting. I have to learn how to animate svg first so for this I need a little time to learn.

This could downstream to mapknitter too, but if we needed a quick implementation I would have gone with a leaflet-spin. But this wouldn't look that great. the old progress bar was much better. I think in MK we should just try to reimplement the old progress bar, in a modal or however you'd like

* update ldi and leaflt-toolbar dependencies get collection group API working custom export action working clean up more cleanup * fix for some /this/ scope issues * selection working * LDI v0.5.2 with correct ordering, timestamp logging * test * removed package-lock.json * completion alert * JSON.parse(data) for returned status * null as non-string * response fix * prompt for scale * change status_url to export_url in test

codeclimate bot reviewed Jul 2, 2019

View reviewed changes

app/assets/javascripts/mapknitter/Map.js Outdated

actions: [Exports]

}).addTo(map);

/** TODO: make an addLayers func */

Copy link

codeclimate bot Jul 2, 2019

Choose a reason for hiding this comment

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

TODO found

jywarren mentioned this pull request Jul 2, 2019

fix for cache bust in multi export publiclab/Leaflet.DistortableImage#318

Merged

sashadev-sky force-pushed the mk-exports branch from 83df207 to 261aca8 Compare July 3, 2019 04:27

update ldi and leaflt-toolbar dependencies

b005236

get collection group API working custom export action working clean up more cleanup

jywarren reviewed Jul 3, 2019

View reviewed changes

app/assets/javascripts/mapknitter/Map.js Outdated Show resolved Hide resolved

jywarren mentioned this pull request Jul 3, 2019

Breaking change to toggleRotateDistort? publiclab/Leaflet.DistortableImage#319

Closed

completion alert

a9b6791

sashadev-sky commented Jul 3, 2019

View reviewed changes

JSON.parse(data) for returned status

411f521

null as non-string

363a095

jywarren mentioned this pull request Jul 3, 2019

better response handling publiclab/Leaflet.DistortableImage#323

Merged

response fix

b429953

jywarren added 3 commits July 3, 2019 17:46

prompt for scale

cd412a3

change status_url to export_url in test

faaea9d

Merge branch 'mk-exports' of github.com:sashadev-sky/mapknitter into …

4bd87fc

…mk-exports

jywarren merged commit 9299d74 into publiclab:main Jul 3, 2019

cesswairimu mentioned this pull request Jul 5, 2019

Panning/zooming issues with new front page map #792

Closed

This was referenced Jul 6, 2019

Display process bar from cloud exporter in regular exporter sidebar #814

Open

Multiple-image-export #693

Open

	actions: mapknitter.imgActionArray(),
	actions: this.imgActionArray(),

Connect LDI exports downstream #782

Connect LDI exports downstream #782

Conversation

sashadev-sky commented Jul 2, 2019

codeclimate bot commented Jul 2, 2019

codecov bot commented Jul 2, 2019 • edited Loading

Codecov Report

codeclimate bot Jul 2, 2019

Choose a reason for hiding this comment

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

jywarren commented Jul 2, 2019

jywarren commented Jul 2, 2019

sashadev-sky commented Jul 2, 2019

sashadev-sky commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

sashadev-sky Jul 3, 2019

Choose a reason for hiding this comment

jywarren Jul 3, 2019

Choose a reason for hiding this comment

jywarren commented Jul 3, 2019

sashadev-sky commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

sashadev-sky commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

sashadev-sky commented Jul 3, 2019

jywarren commented Jul 3, 2019

jywarren commented Jul 3, 2019

sashadev-sky commented Jul 3, 2019 • edited Loading

codecov bot commented Jul 2, 2019 •

edited

Loading

sashadev-sky commented Jul 3, 2019 •

edited

Loading