Multiple-image-export #693

divyabaid16 · 2019-06-13T19:39:40Z

Fixes #659 (<=== 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/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!

app/controllers/images_controller.rb

codeclimate · 2019-06-13T19:40:34Z

Code Climate has analyzed commit 19fcff78 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	2

View more on Code Climate.

codecov · 2019-06-14T08:54:47Z

Codecov Report

Merging #693 into main will decrease coverage by 0.54%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
- Coverage   73.15%   72.61%   -0.55%     
==========================================
  Files          37       37              
  Lines        1356     1362       +6     
==========================================
- Hits          992      989       -3     
- Misses        364      373       +9

Impacted Files	Coverage Δ
app/controllers/maps_controller.rb	`89.47% <0%> (-2.42%)`	⬇️
app/models/map.rb	`92.66% <75%> (-0.54%)`	⬇️
lib/exporter.rb	`91.92% <0%> (-2.25%)`	⬇️

divyabaid16 · 2019-06-14T11:38:42Z

Hi @jywarren @sashadev-sky this is almost done. Please see if it is working for you!

jywarren · 2019-06-19T20:29:52Z

app/controllers/images_controller.rb

@@ -96,6 +96,20 @@ def update
    render text: 'success'
  end

+  def export
+    @warpable = Warpable.find params[:id]


Ah this is cool! I wonder though, if we could try redesigning it so that the export request has a list of warpable ids to export, rather than storing this status in the records here?

So we'd make a change to this method to accept a warpables parameter:

mapknitter/app/controllers/maps_controller.rb

Lines 127 to 135 in 5987398

# run the export

def export

@map = Map.find_by(id: params[:id])

if logged_in? || Rails.env.development? || verify_recaptcha(model: @map, message: "ReCAPTCHA thinks you're not a human!")

render plain: @map.run_export(current_user, params[:resolution].to_f)

else

render plain: 'You must be logged in to export, unless the map is anonymous.'

end

end

So then we'd pass in a new parameter to map.run_export that's warpable_ids, and then use it (with Warpable.find warpable_ids) to start the export instead of relying on placed_warpables.

However we could default back to placed_warpables using:

warpables = Warpable.find(warpable_ids) || placed_warpables

What do you think of that?

The advantage here is that since @sashadev-sky is working on sending export requests off-server to a 3rd party service (which we're working on here https://github.com/publiclab/mapknitter-exporter-sinatra/), we can soon switch this to relying less and less on internal knowledge of our warpables, and just be sending coordinate and image URL data to an off-server system.

I hope this makes sense!!!

jywarren · 2019-06-19T20:35:37Z

Hi @divyabaid16 this is really well thought through -- thanks! I am writing to suggest an alternative approach that makes some different modularity decisions. I hope it makes sense!

jywarren · 2019-06-25T23:08:14Z

Looks like great progress here!!! 🎉 Ping me (and/or reviewers) once it's ready! Keep up the great work!

divyabaid16 · 2019-06-26T08:23:16Z

@jywarren @publiclab/mapknitter-reviewers this is ready for the review.
Thanks!

cesswairimu

Wow awesome work @divyabaid16 🎉 🎉

cesswairimu · 2019-06-26T14:39:13Z

Also a suggestion(not related to this but affects this) , @publiclab/mapknitter-reviewers maybe we could remove the rubocop linter from travis since we have it in code climate?

kaustubh-nair · 2019-06-26T16:09:14Z

Also a suggestion(not related to this but affects this) , @publiclab/mapknitter-reviewers maybe we could remove the rubocop linter from travis since we have it in code climate?

I agree with that too. Let's discuss it here #548

jywarren

This looks awesome, @divyabaid16 -- great work. You'll need these skills and this code familiarity once we start doing deeper integration with off-site exporting, so this is great to see!

I'm sorry it took a while to write back. Is this PR compatible with the "list vs. grid" view toggle PR? #738 i think?

Also, is there a way in the grid view to show which images have been placed on the map, versus have yet to be placed? (it's if the image has a nodes property) -- maybe we show unplaced images in 50% opacity, and show a tooltip (https://getbootstrap.com/docs/4.1/components/tooltips/#examples) that says "this image has not yet been placed on the map" and doesn't allow it to be selected for export?

jywarren · 2019-07-02T14:43:15Z

Let's also plan ahead a bit - eventually, we should have JS event handlers in both DistortableCollection (in https://github.com/publiclab/Leaflet.DistortableImage) and in this UI so that when we select an image in one, it gets selected in the other, and vice versa. This could be a way to unify the off-site export system, what do you think? Let's open a new issue and discuss!

divyabaid16 · 2019-07-02T21:44:43Z

The export part only shows the grid view. I just wanted to show it in a compact form so I just got started with the grid view just like we select the images in the phone we would like to share.
And #738 is independent of export and is just for the image tab.
And I will surely distinguish between the images placed on the map and otherwise.
Thanks!

jywarren · 2019-07-02T21:51:24Z

Great! Sasha had some great follow-up suggestions for labeling order from that PR too. Thanks and thanks for your patience with these!

…

On Tue, Jul 2, 2019, 5:45 PM Divya Baid ***@***.***> wrote: The export part only shows the grid view. I just wanted to show it in a compact form so I just got started with the grid view just like we select the images in the phone we would like to share. And #738 <#738> is independent of export and is just for the image tab. And I will surely distinguish between the images placed on the map and otherwise. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#693?email_source=notifications&email_token=AAAF6J43NZM7DZMCTAYDHSLP5PD4ZA5CNFSM4HX5SWE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZCULLI#issuecomment-507856301>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JY35OVHVTNFRWIAH63P5PD4ZANCNFSM4HX5SWEQ> .

divyabaid16 · 2019-07-04T13:48:23Z

Hi, @jywarren I have looked into account the opacity of the unplaced images and disabled it for export. Now it is ready.
Thank!

jywarren · 2019-07-08T23:33:11Z

Hm, what is failing in the Travis build? Thanks @divyabaid16 !!!

sashadev-sky · 2019-07-08T23:55:03Z

it is just a rubocop linter problem! Testing it out right now :)

sashadev-sky · 2019-07-09T00:05:19Z

@divyabaid16 not sure why but for me it is not allowing me to add a map locally. (The url does not autofill while title is entered or location for lat lng. Is this working for you? Perhaps you need a rebase? I see this is still using the old front page ui

divyabaid16 · 2019-07-09T02:33:57Z

Ya, I will just rebase my PR then it would be ready.

sashadev-sky · 2019-07-09T02:41:14Z

@divyabaid16 ok cool ping me when it is rebased if you'd like me to look again!

divyabaid16 · 2019-07-09T05:12:15Z

Hi @sashadev-sky it's done. But now it's showing the error

mapknitter/app/controllers/export_controller.rb

Line 88 in 9299d74

    
           # mySQL2 error ActiveRecord::StatementInvalid (Mysql2::Error: Field 'bands_string' doesn't have a default value: INSERT INTO `exports` (`export_url`, `created_at`, `updated_at`) VALUES ('//export.mapknitter.org/id/1562102960/status.json', '2019-07-02 21:29:20', '2019-07-02 21:29:20')):

jywarren · 2019-07-09T22:21:51Z

Oh huh - see how Sasha fixed that in #782 I think? Yes -- https://github.com/publiclab/mapknitter/pull/782/files#diff-0d9df75c86ab6387aa553c69b3e0033bR88

I'm also seeing the Rubocop linting error as well! It's very unfriendly, but:

Offenses:
app/models/map.rb:233:5: C: Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.
    if warpable_ids == '' ...
    ^^^^^^^^^^^^^^^^^^^^^
28 files inspected, 1 offense detected
The command "bundle exec rubocop" exited with 1.

Hope that helps!

divyabaid16 · 2019-07-10T08:53:51Z

Hi @jywarren it is fixed. It's the error caused by Codecov here too.
Thanks!

jywarren · 2019-07-10T21:33:28Z

OK, awesome! Do you need a new review on this? Thanks!

jywarren · 2019-07-10T21:34:46Z

Can you update me on the status? And, after merging this shall we shift to #814 and #818 ? Thanks @divyabaid16 -- exciting!

divyabaid16 · 2019-07-11T00:26:10Z

Ya, it was working for me.
This can be merged now.

Thanks!

divyabaid16 · 2019-07-11T00:39:29Z

And just to cross verify and be extra sure, can @jywarren or @sashadev-sky test or review this once?

divyabaid16 · 2019-07-11T06:36:44Z

And regarding the status: currently, the export_url is shown as NULL
The url is not getting created.

jywarren · 2019-07-11T18:48:43Z

Hmm, @sashadev-sky is this what we're supposed to be storing the cloud export status.json URL at? If so it'll only have a value if we're doing a cloud export. If not, it'll be empty!

…

On Thu, Jul 11, 2019 at 2:37 AM Divya Baid ***@***.***> wrote: And regarding the status: currently, the export_url is shown as NULL The url is not getting created. [image: Selection_206] <https://user-images.githubusercontent.com/32747809/61027644-55d61b00-a3d4-11e9-8b0e-e1d060121edc.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#693?email_source=notifications&email_token=AAAF6J22V74KUX7DY7NWXVTP63IH5A5CNFSM4HX5SWE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZVVJWI#issuecomment-510350553>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JZK3ETHXR2V5WKBOGLP63IH5ANCNFSM4HX5SWEQ> .

jywarren · 2019-07-26T16:48:59Z

Hi, folks, can we check in here to get synced up, keeping #298 in mind? Shall we wrap this up, then begin adding an alternate interface for the cloud-based multiple image export? Thanks!

divyabaid16 · 2019-08-25T08:59:16Z

@jywarren @sashadev-sky I rebased my PR.

sashadev-sky

Just rebased and reviewed this - made some comments about it.
You need to rebase but the code is still relevant if you want to fix it!

Also one additional thing, I think this should be synced with if the image is selected via the regular interface (clicked, shift+drag) - then it will appear in the sidebar with a red border as if you had otherwise checked it there. Otherwise, this will cause confusion

sashadev-sky · 2020-11-24T01:48:05Z

app/views/maps/_sidebar_exports.html.erb

+var exp1 = 0;
+// //var arr = [];
+// var b = $('.export-warpable').length;
+//     console.log(b);
+//     var k = [0, 0, 0];
+//     console.log(k);
+$('.export-warpable').click(function() {
+    var warpable_id = $(this).children('img').attr("data-warpable-id");
+    console.log(warpable_id);
+    if(exp1==0){
+      $(this).css('border', "solid 2px red");
+      $(this).parent().addClass('export-image');
+      exp1 = 1;
+    }
+    else{
+      $(this).css('border', "none");
+      $(this).parent().removeClass('export-image');
+      exp1 = 0;
+    }
+});


Just got around to reviewing this! So this code assumes that you're only toggling the border on one image. If you select 1, and then select a 2nd, the 2nd won't get a border unless you click it twice. You could refactor maybe to something like this:

$('.export-warpable').click(function() { if ($(this).css('border')[0] === '0') { $(this).css('border', 'solid 2px red'); $(this).parent().addClass('export-image'); } else { $(this).css('border', 'none'); $(this).parent().removeClass('export-image'); } });

mind the double vs single quotes and { spacing

but better to move the css logic under the export-image class in css, and then all you have to do is toggle the class

remove console.logs and comments

sashadev-sky · 2020-11-24T02:12:57Z

app/views/maps/_sidebar_exports.html.erb

+function check_export(){
+  if($('.export-image').length > 0){
+    $('#begin-export').attr('disabled',false).attr('title','Start Export');
+  }
+  else{
+    $('#begin-export').attr('disabled',true).attr('title','Select images to export');
+  }
+};
+


This function is not being called anywhere, so doesn't work. Also disabled would be a prop and to change the text use html('text')

codeclimate bot reviewed Jun 13, 2019

View reviewed changes

app/controllers/images_controller.rb Outdated Show resolved Hide resolved

app/controllers/images_controller.rb Outdated Show resolved Hide resolved

divyabaid16 changed the title ~~[WIP] Multiple-image-export~~ Multiple-image-export Jun 14, 2019

divyabaid16 requested review from jywarren and sashadev-sky June 17, 2019 05:16

divyabaid16 mentioned this pull request Jun 19, 2019

Selection of multiple images to export #659

Open

2 tasks

jywarren requested changes Jun 19, 2019

View reviewed changes

divyabaid16 force-pushed the multiple-image-export branch from 5a029e9 to d09798b Compare June 25, 2019 13:44

divyabaid16 mentioned this pull request Jun 26, 2019

Weekly community Check-In #26 (Bash Aliases) publiclab/plots2#5943

Closed

cesswairimu approved these changes Jun 26, 2019

View reviewed changes

jywarren mentioned this pull request Jul 2, 2019

Insert custom updater function into Leaflet.DistortableImage for displaying export status #756

Closed

jywarren requested changes Jul 2, 2019

View reviewed changes

jywarren mentioned this pull request Jul 2, 2019

Image ordering in sidebar using sorting options #640

Merged

5 tasks

jywarren mentioned this pull request Jul 6, 2019

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

Open

divyabaid16 force-pushed the multiple-image-export branch from 28ecd36 to ee62333 Compare July 9, 2019 04:31

grvsachdeva added the needs-review label Jul 25, 2019

jywarren mentioned this pull request Jul 26, 2019

fully externalize exporter; untested #530

Closed

jywarren mentioned this pull request Aug 9, 2019

multiple image cloud export only returning one image #919

Closed

divyabaid16 added 8 commits August 25, 2019 14:04

Export improved

cadc9b3

Changes made

e9fc4dc

Changes in unit test made

d52d6fb

Test passing

a72d22b

Opacity to uplaced images added

ae11bae

Error resolved

3582af8

Rubocop fix

d7e55ef

Rubocop fix

e0f73c8

divyabaid16 force-pushed the multiple-image-export branch from 08eac7e to e0f73c8 Compare August 25, 2019 08:35

sashadev-sky requested changes Nov 24, 2020

View reviewed changes

	# run the export
	def export
	@map = Map.find_by(id: params[:id])
	if logged_in? \|\| Rails.env.development? \|\| verify_recaptcha(model: @map, message: "ReCAPTCHA thinks you're not a human!")
	render plain: @map.run_export(current_user, params[:resolution].to_f)
	else
	render plain: 'You must be logged in to export, unless the map is anonymous.'
	end
	end

Multiple-image-export #693

Are you sure you want to change the base?

Multiple-image-export #693

Conversation

divyabaid16 commented Jun 13, 2019 • edited Loading

codeclimate bot commented Jun 13, 2019

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

divyabaid16 commented Jun 14, 2019

jywarren Jun 19, 2019

Choose a reason for hiding this comment

jywarren Jun 19, 2019

Choose a reason for hiding this comment

jywarren commented Jun 19, 2019

jywarren commented Jun 25, 2019

divyabaid16 commented Jun 26, 2019

cesswairimu left a comment

Choose a reason for hiding this comment

cesswairimu commented Jun 26, 2019

kaustubh-nair commented Jun 26, 2019

jywarren left a comment

Choose a reason for hiding this comment

jywarren commented Jul 2, 2019

divyabaid16 commented Jul 2, 2019

jywarren commented Jul 2, 2019 via email

divyabaid16 commented Jul 4, 2019

jywarren commented Jul 8, 2019

sashadev-sky commented Jul 8, 2019

sashadev-sky commented Jul 9, 2019

divyabaid16 commented Jul 9, 2019

sashadev-sky commented Jul 9, 2019

divyabaid16 commented Jul 9, 2019

jywarren commented Jul 9, 2019

divyabaid16 commented Jul 10, 2019

jywarren commented Jul 10, 2019

jywarren commented Jul 10, 2019

divyabaid16 commented Jul 11, 2019 • edited Loading

divyabaid16 commented Jul 11, 2019

divyabaid16 commented Jul 11, 2019

jywarren commented Jul 11, 2019 via email

jywarren commented Jul 26, 2019

divyabaid16 commented Aug 25, 2019

sashadev-sky left a comment

Choose a reason for hiding this comment

sashadev-sky Nov 24, 2020

Choose a reason for hiding this comment

sashadev-sky Nov 24, 2020

Choose a reason for hiding this comment

divyabaid16 commented Jun 13, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019 •

edited

Loading

divyabaid16 commented Jul 11, 2019 •

edited

Loading