WIP: Elevation Configuration #56

marczhermo · 2019-06-03T09:14:31Z

No description provided.

codecov-io · 2019-06-03T09:15:59Z

Codecov Report

Merging #56 into primary will decrease coverage by 0.57%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             primary      #56      +/-   ##
=============================================
- Coverage      98.71%   98.13%   -0.58%     
- Complexity       610      612       +2     
=============================================
  Files             45       45              
  Lines           1708     1718      +10     
=============================================
  Hits            1686     1686              
- Misses            22       32      +10

Impacted Files	Coverage Δ	Complexity Δ
src/Admins/SearchAdmin.php	`23.07% <0.00%> (-76.93%)`	`3.00 <2.00> (+2.00)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e420742...77a4b04. Read the comment docs.

Firesphere · 2019-06-03T10:07:58Z

Yash, codecov.io is working again!

Firesphere · 2019-06-03T10:09:09Z

I've added the Criteria class, so you could use that for elevation probably :)

Firesphere · 2019-06-08T03:02:11Z

Could you please rebase?

Firesphere · 2019-08-11T05:18:01Z

Needs a rebase :)

ssmarco · 2019-08-19T08:56:31Z

Not ready yet :)

Firesphere · 2019-08-20T06:55:09Z

src/Admins/SearchAdmin.php

@@ -12,13 +16,34 @@
 class SearchAdmin extends ModelAdmin
 {
    private static $managed_models = [
-        SearchClass::class
+        SearchClass::class,


Please remove SearchClass for now?

Firesphere · 2019-08-20T06:55:27Z

src/Admins/SearchAdmin.php

-        SearchClass::class
+        SearchClass::class,
+        Elevation::class,
+        ElevatedItem::class,


Shouldn't ElevatedItem be managed through Elevation?

Firesphere · 2019-08-20T06:56:14Z

src/Admins/SearchAdmin.php

+
+            $gridField
+                ->getConfig()
+                ->addComponent(new GridFieldOrderableRows('Rank'));


Maybe also add inline editing if it's possible? Saves users from clicking "new" etc.

Firesphere · 2019-08-20T06:56:58Z

src/Models/ElevatedItem.php

+        'Keywords' => Elevation::class,
+    ];
+
+    private static $summary_fields = ['Title', 'Rank', 'ObjectClass', 'SolrID', 'Include', 'Exclude'];


Please make it a list 😄

Also, I think ObjectClass should be replaced with a method, that returns the Singular name of the class. A bit more friendly :)

Firesphere · 2019-08-20T06:57:28Z

src/Models/ElevatedItem.php

+    private static $table_name = 'ElevatedItem';
+
+    private static $db = [
+        'Rank' => 'Int',


Can you stick to my standard of aligning the =>? Sorry, I'm really picky 😄

Firesphere · 2019-08-20T06:58:21Z

src/Models/Elevation.php

+        'Items' => ElevatedItem::class,
+    ];
+
+    private static $summary_fields = ['ID', 'Keyword'];


As above, it would be good to list them out instead of using as a string.

Also, I'd add Items.Count, so people can see how many items are elevated by this keyword without having to look deeper.

Firesphere · 2019-08-20T06:59:30Z

Solr/5/extras/solrconfig.xml

@@ -810,7 +810,7 @@
        <lst name="defaults">
            <str name="echoParams">explicit</str>
            <int name="rows">10</int>
-            <str name="df">_text</str>
+            <str name="df">text</str>


The rename is good, but I'm mildly afraid it might clash at some point with the Text DBObject from SilverStripe, causing confusion in naming etc.. Do you have any thoughts about that?

Firesphere · 2019-08-20T07:02:51Z

Final comment, instead of using the elevate component from Solr, could you use the "At query time" elevation for this? It's a combination.

It would most likely make the code a lot clearer, having an buildElevation method on query time, compared to switching to the elevate endpoint.

Solarium docs around it are here:
https://solarium.readthedocs.io/en/stable/queries/select-query/building-a-select-query/components/query-elevation-component/

Given that currently we use the /select query everywhere, letting Solarium decide either based on the XML (thus Solr) or the query, is a more readable approach.

Firesphere · 2019-08-20T07:03:59Z

src/Models/ElevatedItem.php

+        'SolrID' => 'Varchar(255)',
+        'Include' => 'Boolean(1)',
+        'Exclude' => 'Boolean(0)',
+    ];


Very thorough $db settings. I like!

Firesphere · 2019-08-20T07:04:32Z

src/Models/ElevatedItem.php

+        'ObjectClass' => 'Varchar(255)',
+        'ObjectID' => 'Int',
+        'SolrID' => 'Varchar(255)',
+        'Include' => 'Boolean(1)',


For readability, I prefer using Boolean(true) and Boolean(false) over 1 and 0

Firesphere · 2019-09-19T06:52:39Z

What is the current status?
I'm pondering, if you don't have enough time @marczhermo , we could push it to a release after 1.0?

marczhermo · 2019-09-19T07:10:03Z

Apologies @Firesphere, got distracted by a lot of things and React. I suggest after 1.0 release 😊

Firesphere · 2019-09-22T06:27:13Z

Coolbeans. It's not a heavy requirement to match FTS anyway :)

Firesphere · 2019-10-27T22:05:44Z

You might need to update the tests a bit :)

Firesphere · 2019-10-31T05:24:57Z

Woah! Did you get the React components working?! Really keen to see that in action!

marczhermo · 2019-10-31T05:45:07Z

Still a work in progress but yeah, the Search component is utilized as part of the implementation 😁 and more to come

codeclimate · 2020-03-04T10:55:59Z

src/Admins/SearchAdmin.php

@@ -57,4 +64,23 @@ public function init()

        Requirements::css('firesphere/solr-search:client/dist/main.css');
    }
+
+    public function getEditForm($id = null, $fields = null)


Avoid variables with short names like $id. Configured minimum length is 3.

codeclimate · 2020-03-04T10:56:00Z

src/Models/ElevatedItem.php

+
+use SilverStripe\ORM\DataObject;
+
+class ElevatedItem extends DataObject


The property $table_name is not named in camelCase.

codeclimate · 2020-03-04T10:56:00Z

src/Models/ElevatedItem.php

+
+use SilverStripe\ORM\DataObject;
+
+class ElevatedItem extends DataObject


The property $belongs_many_many is not named in camelCase.

codeclimate · 2020-03-04T10:56:00Z

src/Models/ElevatedItem.php

+
+use SilverStripe\ORM\DataObject;
+
+class ElevatedItem extends DataObject


The property $summary_fields is not named in camelCase.

codeclimate · 2020-03-04T10:56:01Z

src/Models/ElevatedItem.php

+
+class ElevatedItem extends DataObject
+{
+    private static $table_name = 'ElevatedItem';


Avoid unused private fields such as '$table_name'.

codeclimate · 2020-03-04T10:56:04Z

Code Climate has analyzed commit 77a4b04 and detected 18 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Bug Risk	9
Style	9

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 98.0% (-0.5% change).

View more on Code Climate.

marczhermo force-pushed the sheepy/elevation-configuration branch from 5c6d77e to 8a145b0 Compare June 7, 2019 12:37

marczhermo force-pushed the sheepy/elevation-configuration branch from 8a145b0 to bdeab0c Compare June 8, 2019 06:31

marczhermo force-pushed the sheepy/elevation-configuration branch from bdeab0c to bcf7bc6 Compare August 19, 2019 08:55

Firesphere added the enhancement New feature or request label Aug 20, 2019

Firesphere mentioned this pull request Aug 20, 2019

Add Elevation #23

Closed

Firesphere requested changes Aug 20, 2019

View reviewed changes

Firesphere reviewed Aug 20, 2019

View reviewed changes

Firesphere force-pushed the master branch 7 times, most recently from 83712cb to 69471d3 Compare September 9, 2019 02:43

marczhermo force-pushed the sheepy/elevation-configuration branch 3 times, most recently from 01e90e5 to 8e3dacc Compare September 13, 2019 01:35

Firesphere added this to the 1.0 milestone Sep 19, 2019

Firesphere force-pushed the sheepy/elevation-configuration branch from 8e3dacc to b64b41d Compare October 20, 2019 10:19

marczhermo force-pushed the sheepy/elevation-configuration branch from b64b41d to 6f9357b Compare October 27, 2019 05:52

Firesphere force-pushed the master branch 2 times, most recently from 56f61e2 to aa44e02 Compare December 27, 2019 01:20

Firesphere assigned marczhermo Jan 1, 2020

Marco Hermo and others added 2 commits March 4, 2020 23:31

Elevation: Change field name to text worked for elevation

65d65c9

Model: Elevation and Items

77a4b04

marczhermo force-pushed the sheepy/elevation-configuration branch from f8fadb0 to 77a4b04 Compare March 4, 2020 10:50

codeclimate bot reviewed Mar 4, 2020

View reviewed changes

Firesphere added the hacktoberfest label Oct 3, 2020

Firesphere closed this Oct 4, 2020

Firesphere reopened this Oct 4, 2020

Firesphere changed the base branch from master to primary October 4, 2020 04:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Elevation Configuration #56

WIP: Elevation Configuration #56

marczhermo commented Jun 3, 2019

codecov-io commented Jun 3, 2019 •

edited by codecov bot

Loading

Firesphere commented Jun 3, 2019

Firesphere commented Jun 3, 2019

Firesphere commented Jun 8, 2019

Firesphere commented Aug 11, 2019

ssmarco commented Aug 19, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere commented Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere Aug 20, 2019

Firesphere commented Sep 19, 2019

marczhermo commented Sep 19, 2019

Firesphere commented Sep 22, 2019

Firesphere commented Oct 27, 2019

Firesphere commented Oct 31, 2019

marczhermo commented Oct 31, 2019

codeclimate bot Mar 4, 2020

codeclimate bot Mar 4, 2020

codeclimate bot Mar 4, 2020

codeclimate bot Mar 4, 2020

codeclimate bot Mar 4, 2020

codeclimate bot commented Mar 4, 2020


		use SilverStripe\ORM\DataObject;

		class ElevatedItem extends DataObject

WIP: Elevation Configuration #56

Are you sure you want to change the base?

WIP: Elevation Configuration #56

Conversation

marczhermo commented Jun 3, 2019

codecov-io commented Jun 3, 2019 • edited by codecov bot Loading

Codecov Report

Firesphere commented Jun 3, 2019

Firesphere commented Jun 3, 2019

Firesphere commented Jun 8, 2019

Firesphere commented Aug 11, 2019

ssmarco commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere commented Sep 19, 2019

marczhermo commented Sep 19, 2019

Firesphere commented Sep 22, 2019

Firesphere commented Oct 27, 2019

Firesphere commented Oct 31, 2019

marczhermo commented Oct 31, 2019

codeclimate bot Mar 4, 2020

Choose a reason for hiding this comment

codeclimate bot Mar 4, 2020

Choose a reason for hiding this comment

codeclimate bot Mar 4, 2020

Choose a reason for hiding this comment

codeclimate bot Mar 4, 2020

Choose a reason for hiding this comment

codeclimate bot Mar 4, 2020

Choose a reason for hiding this comment

codeclimate bot commented Mar 4, 2020

codecov-io commented Jun 3, 2019 •

edited by codecov bot

Loading