WIP: Formatting and improving map popups

[nstjean]

On /map we now have people displaying from the PLpeople layer in LEL, displayed via LeafletBlurredLocationDisplay.

We also have all the site content being displayed leaflet_helper.js. This PR updates the styling of those popups.

[codecov]

Codecov Report

Merging #7562 into master will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7562      +/-   ##
==========================================
- Coverage   82.03%   81.94%   -0.09%     
==========================================
  Files          97       97              
  Lines        5615     5627      +12     
==========================================
+ Hits         4606     4611       +5     
- Misses       1009     1016       +7     

Impacted Files	Coverage Δ
app/api/srch/search.rb	66.24% <100.00%> (-3.04%)	⬇️
app/models/doc_result.rb	100.00% <100.00%> (ø)
app/services/execute_search.rb	88.88% <0.00%> (-5.56%)	⬇️

[nstjean]

This is how they look on inline maps - currently they are styled the same but everything can easily be overridden for inline maps if we want to adjust the size of them.

[nstjean]

Question: On the popups do we want to display the location lat and lon? I can also display other information about the post if we want to.

I tried to display the number of comments but that caused an error because it depends on if the person viewing the page is a moderator/admin or not. The API doesn't like doing calculations based on current user, it causes an error.

[nstjean]

Added the # link for wikis to link to their /map/SLUG

[nstjean]

I don't know what's up with the tests... it's having failures and timeouts on things I didn't touch.

[cesswairimu]

Restarting travis

[alaxalves]

@nstjean Whats the output when you locally run the system tests? I had to suppress the logs for it in Travis with > /dev/null because it could not support their size.

[nstjean]

I get some failures when I run the system tests locally, but it does run properly. It looks like travis isn't even starting the tests.

[cesswairimu]

🤔 Taking a look

[jywarren]

Question: On the popups do we want to display the location lat and lon? I can also display other information about the post if we want to.

I think not, it seems extraneous!

This is looking awesome though!!!

[alaxalves]

I get some failures when I run the system tests locally, but it does run properly. It looks like travis isn't even starting the tests.

It seems it doesnt run because of the suppressed logs, but the system tests run properly on Travis. The problem was that the Travis machine could not support to output the amount of logs we produce (mostly byte-represented screenshot images). Use your local environment to fix the system tests and the Travis pipeline should get fixed.

[jywarren]

Thanks for the suggestion, @alaxalves !!!

[jywarren]

Taking a closer look at this now...

[jywarren]

OK, 11 days ago this change was made, and that seems to have added the > /dev/null to the system tests line: #7568

Looking deeper...

[jywarren]

So @alaxalves you're saying that we can't see the error because the output of script: bundle exec rails test:system > /dev/null is being dumped to /dev/null? Why was it not an issue before? And, could we store it but then filter out the images before outputting it to the Travis log?

[jywarren]

Would one possible step be to remove the > /dev/null temporarily in this PR to try to get a look at the error output?

[alaxalves]

So @alaxalves you're saying that we can't see the error because the output of script: bundle exec rails test:system > /dev/null is being dumped to /dev/null? Why was it not an issue before? And, could we store it but then filter out the images before outputting it to the Travis log?

Sorry for late reply Jeff,
Yes, the error log is being dumped to /dev/null since Travis could handle the log size and crashed every build. I think that it has not been an issue before because we had been using Docker to internally run our tests, and Docker might have been suppressing the error messages.

[nstjean]

So is there anything I can do here? @jywarren @alaxalves

[jywarren]

Hmm. Let me think -- wouldn't some of our system tests have failed if that were happening? I'm going to look at the commits/test runs leading up to the change to see if I can learn more...

[jywarren]

A number of test failures predated this but presumably they all passed at some point but not after the merge (when they get re-run). I remember we were having some inconsistent test results and rebooting them had seemed to work. I'm now going back to the most recent one that did work:

#7520

[jywarren]

We were getting this error: https://travis-ci.org/github/publiclab/plots2/jobs/653371760#L3915

ERROR["test_removing_tags_from_the_post", #<Minitest::Reporters::Suite:0x00007f62dc8433e8 @name="PostTest">, 93.41379365400002]
 test_removing_tags_from_the_post#PostTest (93.41s)
Minitest::UnexpectedError:         Mysql2::Error: 
            app/controllers/tag_controller.rb:396:in `delete'

which could be a disk space error...

[jywarren]

Other errors include lots of:

Minitest::UnexpectedError:         Selenium::WebDriver::Error::TimeoutError: timeout: Timed out receiving message from renderer: 10.000
          (Session info: headless chrome=75.0.3770.142)
          (Driver info: chromedriver=74.0.3729.6 (255758eccf3d244491b8a1317aa76e1ce10d57e9-refs/branch-heads/3729@{#29}),platform=Linux 4.15.0-1028-gcp x86_64)
            test/system/search_test.rb:24:in `block in <class:SearchTest>'

which could also be disk space related. I'm going to try removing the /dev/null portion to see if we can get it to intermittently pass.

Then we should consider ways to not log the screenshots, rather than dumping the whole log.

[jywarren]

So, the screenshots don't seem to be getting logged:

[Screenshot]: tmp/screenshots/test_front_page_with_navbar_search_autocomplete.png
[Screenshot]: tmp/screenshots/test_searching_an_item_from_the_homepage.png
[Screenshot]: tmp/screenshots/test_viewing_the_dashboard.png
[Screenshot]: tmp/screenshots/test_viewing_the_dashboard.png
[Screenshot]: tmp/screenshots/test_viewing_the_settings_page.png
[Screenshot]: tmp/screenshots/test_viewing_question_post.png
[Screenshot]: tmp/screenshots/test_simple-data-grapher_powertag.png
[Screenshot]: tmp/screenshots/test_blog.png

however they are getting saved. If disk space is the issue, could we delete them each after they're uploaded? I would have thought /tmp/ files would get auto deleted... but not sure about that.

[jywarren]

@alaxalves - what exactly did you see about the log size being too big? Is it storage space, or something else? https://travis-ci.org/github/publiclab/plots2/jobs/651968744 doesn't seem too wildly long - system tests logs are about 3k lines. Is that breaking something?

[jywarren]

Aha - ok, https://travis-ci.org/github/publiclab/plots2/jobs/663629236#L1865 does show the data-urls inline. its giant, and got:

The job exceeded the maximum log length, and has been terminated.

But, based on comparison to previous logs, i don't think we used to put these into logs. Looking at where that happens and if we can suppress it just for the screenshots...

[jywarren]

Hmm. I can't see where system test output is configured. When originally installed in https://github.com/publiclab/plots2/pull/4888/files and https://github.com/publiclab/plots2/pull/5868/files we don't seem to be customizing output. Let's see if capybara has a config to customize the log output...

[jywarren]

I'm also not convinced this is a data-url:

�]1337;File=name=dGVzdF9ibG9nLnBuZw==;height=400px;inline=1:iVBORw0KGgoAAAANSUhEUgAABXgAAAV4CAYAAAAaC7U3AAAAAXNSR0IArs4c6QAAIABJREFUeJzs3XdcVHe+//E3MPTeBAGxUCwINlTsGmM0ppje2+5mk2ySm7ubbffu3m3Z397d7O69d7MlvfdimibGqDH2gmLDCoKA0ofemYH5/YFMREBBgeGY1/PxyCPTzjmf+TJOeX+/5/t1Sp453yYAAAAAAAAAgOE4O7oAAAAAAAAAAMCFIeAFAAAAAAAAAIMi4AUAAAAAAAAAgyLgBQAAAAAAAACDIuAFAAAAAAAAAIMi4AUAAAAAAAAAgyLgBQAAAAAAAACDIuAFAAAAAAAAAIMyOboAAAAAYDBxdXXTkPBwBYeEyMvL29HlXJT6+jqVmc0qKSqSxdLcL8e4lNrrQg1EOwMAAHTHKXnmfJujiwAAAAAGA1dXN8WPGaOqykqVmc1qaKh3dEkXxdPTS8EhIfIPCFDG0aN9Hj5eau11ofq7nQEAAM6FgBeXPH9/f82ZM0fJycmKiIhwdDnAoBQVFiI/Px9Hl9FBRWWlNmzcrFffeEtlZeWOLgfAt0TksGg5STp1Ms/RpfSp/npel2p7XSjaAwAAOAJTNOCS5u/vr0cffVRBAX6Kjxmp8LBQR5cEDEpNzRZHl9BJYECArl92jebPna37vv8DQl4AAyI4JESZR486uow+V242K27MmD4PHi/V9rpQ/dXOAAAA58Iia7ikzZkzRyFBAZo7cxrhLmBQgYGB+u69dzu6DADfEl5e3pfkNAMNDfX9Mj/updpeF6q/2hkAAOBcCHhxSUtOTtbY+FhHlwHgIs2eNcPRJQAAAAAAMCgR8OKSFhERoeCgAEeXAeAiBQcFOboEAAAAAAAGJQJe

[jywarren]

dGVzdF9ibG9nLnBuZw== is test_blog.png in base-64, so for some reason we are dumping a file, it seems, to log. When did this begin?

[jywarren]

Hmm, yeah - even the last PR before the #7568 changes does not try to output the files into the log: https://travis-ci.org/github/publiclab/plots2/jobs/654281714#L3717 although it does fail for some reason that could be related or not...?

[jywarren]

So, in summary, before #7568 we were not outputting files into the logs, and the logs were reasonable length. Some other issue may have been causing intermittent test failures but it's not clear to me what the connection is unless it's disk space.

If we can figure out why it's trying to output files into logs, we can probably solve this?

[jywarren]

Hmm, i'm pretty stuck here, i think. @alaxalves maybe after i documented some of this research and digging, you have any new insights? Thank you!!!

[cesswairimu]

@nstjean could you try rebasing see if travis passes

[alaxalves]

Hey @jywarren @cesswairimu Could you guys sum up the discussion here?

[jywarren]

Hi @alaxalves -- thanks, I think my last comment is unfortunately still the latest we know:

For some reason the new setup outputs screenshot files to the logs, but I confirmed that this wasn't the case before #7568, and so the goal is to figure out /why/ this is happening in the new tests setup and stop them from being logged, so that we can see the system tests log output properly and get this PR and others merged. Thanks!

[nstjean]

@nstjean could you try rebasing see if travis passes

Ok, pushing it up to see!

[alaxalves]

@nstjean @jywarren I have fixed this PR here #7715. I had to bypass some test asserts, but Natalie could work on those later. The problem was that we have a method named take_screenshot and it would output the screenshot binaries, now I have configured it to only output the screenshot path.