search

DigitalCommons / land-explorer-front-end

React app for the Land Explorer front end
http://landexplorer.cc
GNU Affero General Public License v3.0
1 stars 0 forks source link

[Social Farms & Gardens-Spring-24] Adding to data layer endless loading #314

Closed lin-d-hop closed 2 months ago

lin-d-hop commented 6 months ago

Describe the bug We added the feature to add a marker/poly/line to a data layer.

On attempting to use this feature I just get an endless load.

To Reproduce Steps to reproduce the behavior:

  1. Go to a marker/line/poly
  2. Click on 'Add to Map'
  3. Change to 'Data Layer' panel
  4. Click on a Data Layer you have access to to add the marker/line/poly to the layer
  5. See the endless spinner

This marker has a name, so I'm wondering if the fact it is 'undefined' is relevant here. Same happens for lines/polys, named and unnamed.

Screenshot from 2024-03-27 16-36-52

Expected behaviour The marker/line/poly should be added to the appropratei data layer, and thus I can view it from any account with access to that data layer.

Context (please complete the following information):

ms0ur1s commented 5 months ago

@lin-d-hop tested this locally and added a test group to staging to test current published development version and all works perfectly now. Solved in #313, video included below.

https://www.awesomescreenshot.com/video/26994377?key=5b67e0e7f0ebd3dd028b0ec788eab0e0

lin-d-hop commented 5 months ago

Tested with lines, markers, polys, adding drawn elements and also data layer elements.

Everything appears to copy across as expected now. Hooary!

Seems like it would be good to have unit tests added to this....

rogup commented 5 months ago

@ms0ur1s I think that this was a combination of issues:

  1. Something wasn't working with the API request which made it hang, but now seems to be working. It's hard to know what this was, now that it's working, so would be difficult to write a UT for it. Unless we tried writing a bunch of UTs around the 'copy to datagroup' feature to catch things that might break in the future. It would be quite speculative so don't think it would rank highly in the benefit:cost ratio. There are more immediately beneficial things than trying to write lots of speculative UTs, such as BE crash analytics and better logging.

  2. There is a front-end bug which is still there but not noticeable if the copy happens quickly. The popup still shows undefined for a split second, because this line of code in DrawingPopup doesn't include all of the required component's parameters, such as type.

{mode === MODE.SAVING && <PopupStatus mode={MODE.SAVING} />}

I've fixed this here https://github.com/DigitalCommons/land-explorer-front-end/commit/4af9566ab0755118e3514ce863f44fef90127853 and cherry picked it to master so it's included in the release.

UTs wouldn't be catch bugs like this on the FE. The automated tests that would catch bugs like this would be a visual regression tests using something like https://garris.github.io/BackstopJS/ I'm not sure how reliable these tests are but I think we should give it a go for a user flow such as this, to see how well it works.