Problem/Motivation

A proposal "Creating a great geospatial search experience in D8" has been selected in Google Summer of Code'2017. Initially I, dbjpanda under the guidance of Nick_vh have coded some part of the module. But later on we decided to shift to Drupal issue queue to work collaboratively with other developers. Kindly review the patch. Basically we had focused on below issues in the last few weeks:

  • Submitted tests for LocationDataType plugin and Raw Plugin
  • Added necessary configuration objects to search_api_location_views_schema.yml
  • Fixed some miscellaneous error as per coding standards
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dbjpanda created an issue. See original summary.

dbjpanda’s picture

Status: Active » Needs review
FileSize
4.1 KB

Kindly review the patch.

borisson_’s picture

Status: Needs review » Needs work

I had reviewed this earlier, but the dreditor plugin is a lot easier for reviewing compared to the github UI. So I found some extra nits to pick. Otherwise this looks great!

  1. +++ b/search_api_location_views/config/schema/search_api_location_views.schema.yml
    @@ -0,0 +1,60 @@
    +          label: 'Map Radius Border Color'
    ...
    +          label: 'Map Radius Border Weight'
    ...
    +          label: 'Map Radius Background Color'
    ...
    +          label: 'Map Radius Background Transparency'
    ...
    +          label: 'Map Radius marker image'
    

    I don't understand the usage of capitals here, they are also inconsistent. Let's make this into "Word word word"? So "Map radius border color", "Map radius marker image", ...

  2. +++ b/search_api_location_views/config/schema/search_api_location_views.schema.yml
    @@ -0,0 +1,60 @@
    +      label: 'Values - it depends on the plugins!!!'
    

    This doesn't look very friendly, let's change this to "A mapping of values" or something like that?

  3. +++ b/tests/src/Kernel/LocationDataTypeTest.php
    @@ -0,0 +1,29 @@
    +class LocationDataTypeTest extends KernelTestBase {
    +  /**
    

    Needs a blank line between these 2.

  4. +++ b/tests/src/Kernel/LocationDataTypeTest.php
    @@ -0,0 +1,29 @@
    +	'user',
    +	'search_api',
    +	'search_api_location'
    +	];
    

    Please use 2 spaces instead of tabs here. If the IDE/editor that you use supports the .editorconfig format that should've been set automatically. You can find plugins for most editors online. Otherwise, just setting your ide to use 2 spaces to indent should be sufficient.

  5. +++ b/tests/src/Kernel/LocationDataTypeTest.php
    @@ -0,0 +1,29 @@
    +  public function testGetValue() {
    

    Should have a doc comment here.

  6. +++ b/tests/src/Kernel/RawTest.php
    @@ -0,0 +1,37 @@
    +	'user',
    +	'search_api',
    +	'search_api_location'
    +	];
    

    /s/tabs/2 spaces/

dbjpanda’s picture

Status: Needs work » Needs review
FileSize
4.17 KB

@borisson_ Rectified as per your comment. Kindly review.

dbjpanda’s picture

FileSize
4.17 KB
dbjpanda’s picture

FileSize
4.15 KB

Ignore patch #4,5 . And review this one please. Thank you.

borisson_’s picture

+++ b/tests/src/Kernel/RawTest.php
@@ -0,0 +1,37 @@
+  public static $modules = [
+  'user',

I said this wrong, sorry. Indentation should happen with 2 spaces at a time, so in this case that's like this, replacing the underscores with spaces. I used underscores to more clearly indicate what I mean.

__public
____'user'

PS: Next time, please provide an interdiff as well as a patch to make reviewing easier.

dbjpanda’s picture

borisson_ Can you submit the patch rectifying it. I really did not get you.

dbjpanda’s picture

FileSize
4.16 KB

Are you telling like this?

borisson_’s picture

FileSize
893 bytes
4.15 KB

Patch + interdiff attached, I hope this makes it clearer.

dbjpanda’s picture

FileSize
4.16 KB
4.16 KB

Yes, I had done that as well. Review the latest patch (second patch) attached in this comment. Ignore the first one.
Also if you have reviewed it. Can we mark it as RTBC?

dbjpanda’s picture

Sorry same patches have been uploaded by mistake. Finally review this patch https://www.drupal.org/files/issues/2891943%239_0.patch

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
andypost’s picture

+++ b/tests/src/Kernel/LocationDataTypeTest.php
@@ -0,0 +1,33 @@
+    $this->assertEquals($sut->getValue('POLYGON((1 1,5 1,5 5,1 5,1 1),(2 2,2 3,3 3,3 2,2 2))'),"3,3");
+   }
+   ¶
+}

minor nitpick, could be fixed on commit - trailing white space

dbjpanda’s picture

@andypost You can have a look on latest patch. As I can see there is no trailing space error.

andypost’s picture

@dbjpanda yes, 2 same named files are confusing)

floydm’s picture

By adding the schema.yml the patch 2891943#9.patch on comment #11 fixes Issue #2879632 - Error when saving a view. Yay!

dbjpanda’s picture

@floydim Thanks for reviewing.

  • Nick_vh committed e17d8cc on 8.x-1.x authored by dbjpanda
    Issue #2891943 by dbjpanda, borisson_, andypost, floydm: Adding tests...
Nick_vh’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.