Support from Acquia helps fund testing for Drupal Acquia logo

Comments

minakshiPh created an issue. See original summary.

minakshiPh’s picture

Kindly review the attached patch for "Group By" example.

Thanks!

minakshiPh’s picture

Status: Active » Needs review
minakshiPh’s picture

Assigned: minakshiPh » Unassigned
minakshiPh’s picture

Modified the patch with some corrections

Kindly review
Thanks!

minakshiPh’s picture

corrected the previous patch and interdiff

Kindly review
Thanks!

The last submitted patch, 5: group_by_list-2791397-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: group_by_list-2791397-6.patch, failed testing.

The last submitted patch, 6: group_by_list-2791397-6.patch, failed testing.

Manav’s picture

Assigned: Unassigned » Manav
Manav’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

Hi @minakshi
I have found some issue in your patch. If your are using "groupBy" clause then use aggregate function with it. "The GROUP BY statement is used in conjunction with the aggregate functions to group the result-set by one or more columns." check this link: groupBy.
Attaching modified patch for the same.

Manav’s picture

Assigned: Manav » Unassigned

Status: Needs review » Needs work

The last submitted patch, 11: dbtng_example_needs-2791397-10.patch, failed testing.

Manav’s picture

Assigned: Unassigned » Manav
Status: Needs work » Needs review
FileSize
2.96 KB

I have checked CI error log and found the problem that "Parse error: syntax error, unexpected '[' in /var/www/html/sites/all/modules/examples/dbtng_example/dbtng_example.module on line 634".
Attaching patch again to remove this error.

Manav’s picture

Assigned: Manav » Unassigned
JayKandari’s picture

Status: Needs review » Reviewed & tested by the community

#14 Looks Good. Changing to RTBC.

Mile23’s picture

+++ b/dbtng_example/dbtng_example.module
@@ -388,6 +392,12 @@ function dbtng_example_menu() {
+  $items['examples/dbtng/grouping_list'] = array(
+    'title' => 'Grouping list',
+    'page callback' => 'dbtng_example_grouping_list',
+    'access arguments' => array('access administration pages'),
+    'type' => MENU_LOCAL_TASK,
+  );

Needs a test which logs in a user with the permissions and then tries to visit that path. Just add it to DBTNGExampleUnitTestCase::testUI().

Mile23’s picture

Status: Reviewed & tested by the community » Needs work
arijits.drush’s picture

Assigned: Unassigned » arijits.drush
Status: Needs work » Needs review
FileSize
4.17 KB
SwapS’s picture

Status: Needs review » Needs work

Review comments on patch #19

1) Would be good to use appropriate test user name

 $this->drupalPost('examples/dbtng/add',
+      array(
+        'name'  => 'Arijit',
+        'surname' => 'Sarkar',
+        'age' => 31,
+      ),
+      t('Add')
+    );

2) Also , No need of Access argument here . Isnt solving any purpose here.

  $items['examples/dbtng/grouping_list'] = array(
    'title' => 'Grouping list',
    'page callback' => 'dbtng_example_grouping_list',
    'access arguments' => array('access administration pages'),
    'type' => MENU_LOCAL_TASK,
  );

Cheers
SwapS

arijits.drush’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
SwapS’s picture

Assigned: arijits.drush » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good to me now.
Good for merge

Cheers
SwapS

Manav’s picture

Assigned: Unassigned » Manav
Manav’s picture

I have checked this patch. Everything thing is fine but there is some coding standard issue so attaching patch again. Also attaching screen-shot for last updated patch, which showing the test result against this patch.

Manav’s picture

Thanks arijits.drush and SwapS for this test cases.

Mile23’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks for the screenshot, but we get a pass/fail from the testbot.

Also, an interdiff would be very helpful for review. Saves a lot of time and effort: https://www.drupal.org/documentation/git/interdiff

Attached is an interdiff for #24. It's all out of scope, meaning it doesn't fix the issue we're working on. If you'd like to fix CS errors in this module, please file another issue.

Reviewing #21:

+++ b/dbtng_example/dbtng_example.test
@@ -86,6 +86,21 @@ class DBTNGExampleUnitTestCase extends DrupalWebTestCase {
     $this->assertFieldByXPath("//*[@id='block-system-main']/div/table[1]/tbody/tr/td[4]", "Roe", "Name 'Roe' Exists in advanced list");
+
+	//Test the grouping tab
+	$this->drupalGet('examples/dbtng/grouping_list');
+	$this->assertPattern("/John[td\/<>\w]+2/", "Text 'John' found in table with 2 count");
+	//Now add new entry
+	$this->drupalPost('examples/dbtng/add',
...
+	$this->drupalGet('examples/dbtng/grouping_list');
+	$this->assertPattern("/Some[td\/<>\w]+1/", "Text 'Some' found in table with 1 count");

These lines use tabs instead of spaces, so they're indented incorrectly.

Once we get that done, we're pretty close. :-)

Mile23’s picture

FileSize
8.08 KB

Ewps... Interdiff.

arijits.drush’s picture

Assigned: Unassigned » arijits.drush
arijits.drush’s picture

Status: Needs work » Needs review
FileSize
4 KB
+
+   //Test the grouping tab
+   $this->drupalGet('examples/dbtng/grouping_list');
+   $this->assertPattern("/John[td\/<>\w]+2/", "Text 'John' found in table with 2 count");
+   //Now add new entry
+   $this->drupalPost('examples/dbtng/add',
+      array(
+        'name'  => 'Some',
+        'surname' => 'Anonymous',
+        'age' => 31,
+      ),
+      t('Add')
+    );
+   $this->drupalGet('examples/dbtng/grouping_list');
+   $this->assertPattern("/Some[td\/<>\w]+1/", "Text 'Some' found in table with 1 count");
arijits.drush’s picture

Assigned: arijits.drush » Unassigned
Manav’s picture

Assigned: Unassigned » Manav
Status: Needs review » Reviewed & tested by the community

Patch #29 working fine for me.

Manav’s picture

Assigned: Manav » Unassigned

  • d73ffa7 committed on 7.x-1.x
    Issue #2791397 by minakshiPh, Manav, arijits.drush, Mile23: DBTNG...
valthebald’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x-1.x. Thanks everyone!

Status: Fixed » Closed (fixed)

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