Implementgroup #959

sashadev-sky · 2019-08-22T21:29:08Z

Fixes #919 (<=== Add issue number here)
Fixes publiclab/Leaflet.DistortableImage#386
Fixes #996
Fixes #982
Fixes #703

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!

===========

changes:

setup is now just this.setupMap() and map.addGoogleMutant() from LDI.
we initialize a collection group immediately after map setup with this.setupCollection(), and this is the only time we initialize it. Before our images were ending up in separate collection groups bc we were making a new one each time.
we use the newly added editable option from LDI
updated rotateBy, scaleBy to be called on img now instead of edit since we moved them
fix Puma configuration file:
- set it to run in development mode (ie rails s will run puma). I personally like puma its Rails 5 default need confirmation on this though! @jywarren @alaxalves @kaustubh-nair
- also updated the pidfile path from pidfile app/tmp/pids/puma.pid -> tmp/pids/puma.pid
manually updated db migrations that threw errors for mySQL. Dropped the database through mysql and ran db:create db:migrate to refresh the schema.rb. This has fixed the errors associated with db:migrate rake db:migrate does not work #703 @kaustubh-nair
Apparently these were all broken bc a schema.rb created in Rails 4 has some internal settings that won't work with Rails 5: https://stackoverflow.com/questions/45925383/activerecordnoenvironmentinschemaerror#49651887
- @icarito I think this might fix Papertrail issue (unknown cause) is causing 500 errors #982 because some of the errors I was receiving before updating schema.rb matched

pending:

go back to LDI, make sure that keydown only fires when the first item in path is the map pane, then get rid of sidebar listener here
go back to LDI, save imgBounds for each img, then use imgGroup._bounds here instead of calculations currently implemented
go back to LDI, update image dragging to use the setCorners() API instead of just img._corners =. Then standardize the setCorner() and setCorners() methods to fire edit each time they are triggered. Remove the edit fire from onHandleDrag. Doc this as event for customizing downstream.
- If we have corners covered and dragging, then I don't think we need to track click, mouseup, touchend to decide if image needs to be saved.
in setupEvents(), I use the edit event fired from LDI onHandleDragEnd

    /**
     * TODO: the edit event is fire on handleDragEnd from LDI. This needs to be documented.
     * and maybe change to 'handledragend' or something to be very explicit. this handle 
     * is necessary beyond click / mouseup because you can distort the image without clicking
     * on it.
     */
    L.DomEvent.on(img, {
      edit: mapknitter.saveImage,
    }, img);

notes

popper.js error is still present in console when placing an image but not from this PR
the action cable code for synchronous editing does not work. Throws error in console when trying to iterate over warpables. Also once gets working, will need to be refactored to use the changes created in this PR (fixes event handling, allows feature group)

=======

codeclimate · 2019-08-22T21:29:47Z

Code Climate has analyzed commit 3bdd866c and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

codecov · 2019-08-22T21:33:11Z

Codecov Report

Merging #959 into main will increase coverage by 1.03%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            main     #959      +/-   ##
=========================================
+ Coverage   72.3%   73.34%   +1.03%     
=========================================
  Files         40       40              
  Lines       1394     1388       -6     
=========================================
+ Hits        1008     1018      +10     
+ Misses       386      370      -16

Impacted Files	Coverage Δ
app/controllers/maps_controller.rb	`92.17% <0%> (+2.6%)`	⬆️
app/channels/concurrent_editing_channel.rb	`83.33% <0%> (+83.33%)`	⬆️
app/channels/application_cable/channel.rb	`100% <0%> (+100%)`	⬆️

sashadev-sky · 2019-08-22T21:36:58Z

implementation not done yet - but core part should be fixed

sashadev-sky · 2019-08-22T21:38:54Z

current bug - images are unplacing from map on reload. I think because this ajax call isnt working. Have been receiving an error about since before I made any modifications

saveImage: function () {
    var img = this;
    // reset change state string:
    img._corner_state = JSON.stringify(img._corners);
    // send save request
    $.ajax('/images', {
      type: 'PATCH',
      data: {
        warpable_id: img.warpable_id,
        locked: (img.editing._mode == 'lock'),
        points:
          img.getCorner(0).lng + ',' + img.getCorner(0).lat + ':' +
          img.getCorner(1).lng + ',' + img.getCorner(1).lat + ':' +
          img.getCorner(3).lng + ',' + img.getCorner(3).lat + ':' +
          img.getCorner(2).lng + ',' + img.getCorner(2).lat,
      },
      beforeSend: function (e) {
        $('.mk-save').removeClass('fa-check-circle fa-times-circle fa-green fa-red').addClass('fa-spinner fa-spin')
      },
      complete: function (e) {
        $('.mk-save').removeClass('fa-spinner fa-spin').addClass('fa-check-circle fa-green')
      },
      error: function (e) {
        $('.mk-save').removeClass('fa-spinner fa-spin').addClass('fa-times-circle fa-red')
      }
    })
  },

sashadev-sky · 2019-08-22T21:44:30Z

ok after updating the route to images/update, on reload every image but the last one to be placed will be on the map

bug 2: after placing 2 images on the map, the first one you drag, regardless of placement order, is draggable immediately. the 2nd one will propagate to map it looks like and drag map instead. Then the 2nd time you click it it will drag.

^^ these 2 bugs are fixed here

app/assets/javascripts/mapknitter/Map.js

sashadev-sky · 2019-08-27T23:49:00Z

@jywarren this is ready for review! Can we try pushing this fix and then checking if it works because unstable is down? Fairly certain it will, and at the very least it fixes the feature group and really cleans up Map.js

sashadev-sky · 2019-08-28T06:15:25Z

@jywarren ok so after watching your export demo I realized its obvious its not working when it says "warping 1 of 1" for what we thought should be multiple.. and I can confirm if that's fixed locally.. which it is!

Had to make one small update but this is actually ready now.

jywarren · 2019-08-28T22:15:16Z

OMG this is awesome. It looks like you really dug into it and found another bug at /images/update type: PATCH, so THANK YOU.

I'm headed to Boston tomorrow to inspect the testing servers in person and get them online. In the meantime, if @cesswairimu @kaustubh-nair @alaxalves anyone else @publiclab/mapknitter-reviewers has any availability to test this out, as it's pretty big, I'd love some extra confirmations from local runs! Thank you and we'll aim to get this online in a test server tomorrow, and if that /doesn't/ happen, I'll test locally myself too and we'll merge. Thank you!!!

sashadev-sky · 2019-08-28T23:13:34Z

@jywarren 🚀more changes to come after this (see pending) 👍

I am a little rusty on rails. images/update is not a restful path correct? Why do we use this path? Sorry could just be misunderstanding too! Anyone else who knwos the q pls feel free to answer @divyabaid16 @cesswairimu @alaxalves @kaustubh-nair

sashadev-sky · 2019-08-28T23:26:01Z

@jywarren @alaxalves @kaustubh-nair @gauravano also this popper.js error won't go away but it doesn't prevent any functionality seems like any ideas what to do? Not urgent to solve here.

jywarren · 2019-09-04T21:36:53Z

Hi @sashadev-sky -- just checking if you're stuck here? Wasn't sure if your last comment meant we needed to wait. And now there is a conflict -- let's get that resolved and merge this? Thank you!

sashadev-sky · 2019-09-04T22:04:13Z

@jywarren oh no this has been ready to merge for a while now!! As long as you reviewed it confirmed all functionality. I will rebase right now. The popper.js problem isnt from this PR and I was just asking a conceptual question.

Also please note in publiclab/Leaflet.DistortableImage#425 I fixed an event propagation problem that came up when i started listening for singleclicks for the google mutant layer. Box selector is not currently working right without it.

I would be happy to merge it and bump the version in this PR, or I can just fix in a new one. let m know!

sashadev-sky · 2019-09-10T22:09:01Z

@jywarren this is ready. just pushed to unstable to check it out.

jywarren · 2019-09-10T22:14:25Z

Great! Thank you, this is awesome. Let's let this go to stable, and on Friday Sebastian and I will try publishing it to production. Great work!

sashadev-sky · 2019-09-10T22:44:17Z

@jywarren I made you a little summary page explaining why, in the end, I didnt add any validation hooks or anything besides a new migration to remove the defaults:

sashadev-sky · 2019-09-10T22:47:37Z

this PR was driving me crazy ln because I didn't know we merged #777 so Travis was failing my build on tests that didn't exist for me locally 🤕

jywarren · 2019-09-11T15:13:00Z

Yikes! Sorry about that Sasha, I am hoping we have fewer and fewer big conflicting PRs now that a lot of the heavy lifting is complete. Thank you so much for the summary and I'm glad we got this worked out. Do you think now we've merged the login issue fix, we can publish this to production on Friday? @alaxalves @kaustubh-nair @cesswairimu @divyabaid16 what do you think? I'd like to see if the live synchronous editing comes online then.

…

On Tue, Sep 10, 2019 at 6:48 PM Sasha Boginsky ***@***.***> wrote: this PR was driving me crazy ln because I didn't know we merged #777 <#777> so Travis was failing my build on tests that didn't exist for me locally 🤕 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#959?email_source=notifications&email_token=AAAF6J2G25LHGNMYJUKE3S3QJAPYTA5CNFSM4IOZOCZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MXCCI#issuecomment-530149641>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J5IAIQIBLBGH46JOI3QJAPYTANCNFSM4IOZOCZA> .

cesswairimu · 2019-09-11T16:53:00Z

Yeeih 🙌 production on Friday sounds good.. Great work @sashadev-sky 🎈

alaxalves · 2019-09-11T17:18:28Z

Yikes! Sorry about that Sasha, I am hoping we have fewer and fewer big conflicting PRs now that a lot of the heavy lifting is complete. Thank you so much for the summary and I'm glad we got this worked out. Do you think now we've merged the login issue fix, we can publish this to production on Friday? @alaxalves @kaustubh-nair @cesswairimu @divyabaid16 what do you think? I'd like to see if the live synchronous editing comes online then.
…
On Tue, Sep 10, 2019 at 6:48 PM Sasha Boginsky @.***> wrote: this PR was driving me crazy ln because I didn't know we merged #777 <#777> so Travis was failing my build on tests that didn't exist for me locally — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#959?email_source=notifications&email_token=AAAF6J2G25LHGNMYJUKE3S3QJAPYTA5CNFSM4IOZOCZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MXCCI#issuecomment-530149641>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6J5IAIQIBLBGH46JOI3QJAPYTANCNFSM4IOZOCZA .

Sounds nice to me!!

sashadev-sky · 2019-09-12T01:21:23Z

@jywarren I think will be ready on Friday! Live synchronous editing will not work I am fairly sure. The code itself has a lot of bugs and it throws errors. Also a lot of code smells, it recopied the code in Map.js that was there before I updated it here.

Mainly, heres the most problematic part:

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

Lines 512 to 528 in bdab16c

    
           $.ajax('/images/' + img.warpable_id, { // send save request 
        
             type: 'PATCH', 
        
             data: { 
        
               warpable_id: img.warpable_id, 
        
               locked: (img.editing._mode == 'lock'), 
        
               points: 
        
                 img.getCorner(0).lng + ',' + img.getCorner(0).lat + ':' + 
        
                 img.getCorner(1).lng + ',' + img.getCorner(1).lat + ':' + 
        
                 img.getCorner(3).lng + ',' + img.getCorner(3).lat + ':' + 
        
                 img.getCorner(2).lng + ',' + img.getCorner(2).lat, 
        
             }, 
        
             beforeSend: function (e) { 
        
               $('.mk-save').removeClass('fa-check-circle fa-times-circle fa-green fa-red').addClass('fa-spinner fa-spin') 
        
             }, 
        
             success: function(data) { 
        
               App.concurrent_editing.speak(data); 
        
             },

The data parameter passed to the success function is always just going to be "success". Then in later functions this parameter is assumed to be a warpable.

If i were to try to fix this, how would I test synchronous editing? Not familiar with how this works

* h * h * updates * Fix featuregroup * checkout randomly modified file * Implementation * setupToolbar working * readOnly mode working * remove googleMutant * finalize * group.editing call * fix puma pidfile path, set it for development * review rebase * update schema.rb to fix DB setup and commands like migrate, setup, etc * delete obsolete config file * update example file * readd new line at end of file * remove left behind comment * add new line to file * bump ldi to 0.7.6 * commit data dump * remove db dump * update prod * return puma back to production * add new migration to avoid potential corruption * delete extra whitespace * re-run all edited migrations * update routes * remove defaults and set before_validations * update u to self * fix ||= space * reverse order reversal * bump LDI to 0.7.7 * remove warpable update hook * try now * before * modifications * update * remove before_validation hook for map * update incorrect syntax in tests

ViditChitkara · 2019-12-09T05:35:18Z

The data parameter passed to the success function is always just going to be "success". Then in later functions this parameter is assumed to be a warpable.

Didn't completely get this. I believe data function is necessary only when save request is completed. Only then should we go for the synchronisation part, right? The data which is being sent to the "success function" is required by the speak method...

If i were to try to fix this, how would I test synchronous editing? Not familiar with how this works

When the map is updated, the speak method of concurrent_editing.js is called which requests the sync method of concurrent_editing_channel.rb to broadcast the updated data to the connected users.
The broadcasted data is finally caught by the received function of app/javascripts/channels/concurrent_editing.js
Finally the received function calls the synchronizeData function to update all the fresh data on the map.

ViditChitkara · 2019-12-09T05:41:40Z

     warpable_id: img.warpable_id, 
     locked: (img.editing._mode == 'lock'), 
     points: 
       img.getCorner(0).lng + ',' + img.getCorner(0).lat + ':' + 
       img.getCorner(1).lng + ',' + img.getCorner(1).lat + ':' + 
       img.getCorner(3).lng + ',' + img.getCorner(3).lat + ':' + 
       img.getCorner(2).lng + ',' + img.getCorner(2).lat, 
   },

what may be problematic in this part, is that when the synch editing code was being written the only change in the above that we were using the _corners[x] instead of getCorners(x).

ViditChitkara · 2019-12-09T05:41:53Z

@sashadev-sky what do you think?

sashadev-sky added 10 commits July 25, 2019 07:41

h

58bc692

h

8dcff6a

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

c075966

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

986b49a

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

34cf99d

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

11e11be

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

ba41fb4

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

d398f00

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

1828645

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

95759ad

sashadev-sky requested a review from jywarren August 22, 2019 21:29

sashadev-sky mentioned this pull request Aug 22, 2019

multiple image cloud export only returning one image #919

Closed

codeclimate bot reviewed Aug 23, 2019

View reviewed changes

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

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

8238c6f

sashadev-sky force-pushed the implementgroup branch from 6e08d68 to 739d8c6 Compare August 27, 2019 23:13

sashadev-sky added the needs-review label Aug 27, 2019

sashadev-sky mentioned this pull request Sep 3, 2019

Pixelorigin publiclab/Leaflet.DistortableImage#424

Merged

5 tasks

Merge branch 'main' of https://github.com/publiclab/mapknitter into main

bf84e93

sashadev-sky added 10 commits September 10, 2019 17:49

fix ||= space

3502a20

reverse order reversal

c1c0e1b

bump LDI to 0.7.7

bbf4f51

remove warpable update hook

2b40f59

try now

ce0f2d2

before

446cc25

modifications

974a996

update

b21a192

remove before_validation hook for map

f7d890a

update incorrect syntax in tests

3410ce6

sashadev-sky force-pushed the implementgroup branch from 6eefdbe to 3410ce6 Compare September 10, 2019 21:50

Merge branch 'main' into implementgroup

73125e8

jywarren merged commit 88c6cb4 into publiclab:main Sep 10, 2019

This was referenced Sep 10, 2019

Schema.rb #1006

Open

Schematask #1007

Open

Delete unwanted initializer #987

Closed

jywarren mentioned this pull request Oct 18, 2019

Follow-up fixes to synchronous editing #1097

Open

jywarren mentioned this pull request Jan 14, 2020

Cloud Export integration, saving & display #1192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementgroup #959

Implementgroup #959

sashadev-sky commented Aug 22, 2019 •

edited

Loading

codeclimate bot commented Aug 22, 2019

codecov bot commented Aug 22, 2019 •

edited

Loading

sashadev-sky commented Aug 22, 2019

sashadev-sky commented Aug 22, 2019

sashadev-sky commented Aug 22, 2019 •

edited

Loading

sashadev-sky commented Aug 27, 2019

sashadev-sky commented Aug 28, 2019

jywarren commented Aug 28, 2019

sashadev-sky commented Aug 28, 2019

sashadev-sky commented Aug 28, 2019 •

edited

Loading

jywarren commented Sep 4, 2019

sashadev-sky commented Sep 4, 2019

sashadev-sky commented Sep 10, 2019

jywarren commented Sep 10, 2019

sashadev-sky commented Sep 10, 2019

sashadev-sky commented Sep 10, 2019

jywarren commented Sep 11, 2019 via email

cesswairimu commented Sep 11, 2019 •

edited

Loading

alaxalves commented Sep 11, 2019

sashadev-sky commented Sep 12, 2019

ViditChitkara commented Dec 9, 2019

ViditChitkara commented Dec 9, 2019

ViditChitkara commented Dec 9, 2019

Implementgroup #959

Implementgroup #959

Conversation

sashadev-sky commented Aug 22, 2019 • edited Loading

codeclimate bot commented Aug 22, 2019

codecov bot commented Aug 22, 2019 • edited Loading

Codecov Report

sashadev-sky commented Aug 22, 2019

sashadev-sky commented Aug 22, 2019

sashadev-sky commented Aug 22, 2019 • edited Loading

sashadev-sky commented Aug 27, 2019

sashadev-sky commented Aug 28, 2019

jywarren commented Aug 28, 2019

sashadev-sky commented Aug 28, 2019

sashadev-sky commented Aug 28, 2019 • edited Loading

jywarren commented Sep 4, 2019

sashadev-sky commented Sep 4, 2019

sashadev-sky commented Sep 10, 2019

jywarren commented Sep 10, 2019

sashadev-sky commented Sep 10, 2019

sashadev-sky commented Sep 10, 2019

jywarren commented Sep 11, 2019 via email

cesswairimu commented Sep 11, 2019 • edited Loading

alaxalves commented Sep 11, 2019

sashadev-sky commented Sep 12, 2019

ViditChitkara commented Dec 9, 2019

ViditChitkara commented Dec 9, 2019

ViditChitkara commented Dec 9, 2019

sashadev-sky commented Aug 22, 2019 •

edited

Loading

codecov bot commented Aug 22, 2019 •

edited

Loading

sashadev-sky commented Aug 22, 2019 •

edited

Loading

sashadev-sky commented Aug 28, 2019 •

edited

Loading

cesswairimu commented Sep 11, 2019 •

edited

Loading