We noticed that for some tickets, there is no edit link in the ticket list.
But when you select the ticket, there is an edit tab and it is editable.
To reproduce:
Create a ticket and assign this ticket to another user.
The other user should have the right: 'Storm ticket: edit if assigned to ticket'
But not: 'Storm ticket: edit of user organization', 'Storm ticket: edit all'
For this ticket, there will be no edit link in the ticket list, but it is editable.

I found the reason:
The storm_icon_edit_node call in theme_stormticket_list doesn't get the complete node, the field assigned_nid is missing there.

Patch follows.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kfritsche’s picture

Patch attached.

As you can see, I added an TODO in the view handler.
It is the same problem for views. But it is harded to fix, as we do not have the node object there.
To fix this there, we need to add the needed fields in the construct() method depending on the node->type, which we do not have at this time. It would be expensive, if we do another query there, to get the missing fields, because this would be happening for each item.
Any ideas? Maybe we should split this in another ticket?

I checked all the other calls of storm_icon_edit_node. I noticed in project and task it is done, like i changed it now for tickets. On all the other list pages, this is handled like it is right now. In my opinion we should do it only one way and do not create a fake node there. Any objections?

kfritsche’s picture

Status: Active » Needs review
juliangb’s picture

Perhaps we could set it so that if the fields are also included in the view (don't have to be displayed) they will be used in the access check?

Likely that people won't know to do this though... so will have to think of a way to document it.

kfritsche’s picture

Here a new Patch, which changes the other calls of storm_icon_edit_node to use the given object, instead of creating a new one. And removes a notice in theme_storm_invoice_list.
I think we could commit this and then think about the view problem a bit more.

Perhaps we could set it so that if the fields are also included in the view (don't have to be displayed) they will be used in the access check?

We could do this, i just tested it. We have all other values there, so in general it is possible. If a value is missing, we can set a drupal message for users with 'administer views' permission, to add the stormorganization and assigned field. But at the moment it isn't possible to add the stormorganization_nid and assigned_nid fields, only the _title fields. And I'm not sure, if we should add these or not? Maybe anyone, wants to show the nid and so we should add it anyways?

juliangb’s picture

The icon functions are also used in attributes - will using node_access still work then?

(Also, the test isn't running because the branch test is "stuck" - I've resent it to try to free it up).

kfritsche’s picture

Yes this works and is much more clearer i think.

juliangb’s picture

Status: Needs review » Needs work

I've looked at this again, and have also now tested. Seems to work, and yes, it is cleaner.

CNW for the typo flagged below.

Also a query I had reading through...

+++ b/storminvoice/storminvoice.theme.inc
@@ -88,7 +82,7 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa
 
   $rows = array();
-  $rows[] = array(
+  $row = array(
     array(
       'data' => t('Total to pay'),
       'style' => 'font-weight: bold;',
@@ -98,20 +92,21 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa

@@ -98,20 +92,21 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa
       'style' => 'text-align: right;',
     ),
   );
-  $rows[key(end($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_topay->tax1),
     'style' => 'text-align: right;',
   );
-  $rows[key(end($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_topay->tax2),
     'style' => 'text-align: right;',
   );
-  $rows[key(end($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_topay->total),
     'style' => 'text-align: right;',
   );
+  $rows[] = $row;
 
-  $rows[] = array(
+  $row = array(
     array(
       'data' => t('Total paid'),
       'style' => 'font-weight: bold;',
@@ -121,20 +116,21 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa

@@ -121,20 +116,21 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa
       'style' => 'text-align: right;',
     ),
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_paid->tax1),
     'style' => 'text-align: right;',
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_paid->tax2),
     'style' => 'text-align: right;',
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals_paid->total),
     'style' => 'text-align: right;',
   );
+  $rows[] = $row;
 
-  $rows[] = array(
+  $row = array(
     array(
       'data' => t('Total'),
       'style' => 'font-weight: bold;',
@@ -144,18 +140,19 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa

@@ -144,18 +140,19 @@ function theme_storminvoice_list($header, $invoices, $itemsperpage, $totals_topa
       'style' => 'text-align: right;',
     ),
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals->tax1),
     'style' => 'text-align: right;',
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals->tax2),
     'style' => 'text-align: right;',
   );
-  $rows[end(array_keys($rows))][] = array(
+  $row[] = array(
     'data' => sprintf('%.2f', $totals->total),
     'style' => 'text-align: right;',
   );
+  $rows[] = $row;
 
   $o .= theme('table', $header, $rows);

Is this related to the issue?

+++ b/stormperson/stormperson.admin.inc
@@ -1,4 +1,4 @@
-<?php
+v<?php

Typo?

kfritsche’s picture

Second is a typo.

And the First thing is not related to any issue. I will remove it and create a new issue. Its removes a warning, as php doesn't like "end(array_keys($rows))" in one call. Should be

$keys = array_keys($rows);
$end = end($keys);

or the way I changed it, without the array_keys and end completly. (Firstly add all to an array and append that array to $rows, instead of getting the last key everytime)

kfritsche’s picture

Status: Needs work » Needs review
FileSize
12.46 KB

New Issue for the first thing: #1463246: Invoice: E_STRICT Warnings

Fixed the typo and here a new patch.

Also a new issue for the view handling: #1463288: Views: Add NID fields and fix operation links

juliangb’s picture

Status: Needs review » Reviewed & tested by the community

I tested #4, and #9 answers all my questions from then, so RTBC?

One thing to keep in mind now that 2.x is a "stable" branch - can we add tests to ensure that once fixed, always fixed?

kfritsche’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
126.53 KB

You're totally right with the tests. So I created tests for all lists which tests the permissions and checks if the edit/delete link is available. All needed nodes are created through the form, so I think this increases our test coverage a lot.

Took sometime, but I hope, we can use this further.

Would be nice, some can review the tests, as I've not written so much tests for Drupal yet (shame on me!).

Now it all should work. Also I found some nasty bugs, while I created these tests. All fixed now.
Also a lot of notices and warnings, which I didn't fixed for now.

3050 passes, 0 fails, and 229 exceptions

Status: Needs review » Needs work

The last submitted patch, ticketlist_edit_link-1448764-11.patch, failed testing.

juliangb’s picture

Thanks for pushing with this. It will be really useful to have this level of testing.

A few comments:
- There are a few places with trailing whitespace (dreditor is great at pointing these out).
- Some of the Storm Expense test comments seem to be talking about invoices rather than expenses.
- We should put a space between // and the comment
- The wording "The [TYPE] edit icon appears not on the list" might sound better worded as "The [TYPE] edit icon does not appear on the list".
- Since amounts are stated when creating the invoices - might be worth asserting that the tax and totals have been calculated correctly.
- More comments would be really helpful in the tests - to quickly judge whether we're asserting the right thing etc. (helps with checking).

- A couple of bits where I wasn't sure about the reasoning behind the changes:

+++ b/stormproject/stormproject.module
@@ -180,7 +180,7 @@ function stormproject_access_sql($sql, $where = array()) {
-      $belonged_teams = stormteam_user_return_teams($account);
+      $belonged_teams = stormteam_user_return_teams();

What's the context behind this change?

+++ b/stormteam/stormteam.module
@@ -514,7 +514,9 @@ function stormteam_list() {
-  $s = "SELECT DISTINCT(n.nid), n.title, n.type, nre.teaser FROM {node} AS n INNER JOIN {stormteam} as ste ON n.vid=ste.vid INNER JOIN {node_revisions} as nre ON n.vid = nre.vid
+  $s = "SELECT n.nid, n.title, n.type, nre.teaser, nre.format, n.uid FROM {node} AS n
+    INNER JOIN {node_revisions} as nre ON n.vid = nre.vid
+    LEFT JOIN {stormteam} AS ste ON n.vid = ste.vid

What's the context behind this change?

kfritsche’s picture

Status: Needs work » Needs review
FileSize
133.78 KB

New Patch fixes trailing whitespaces, changed invoice comments in expenses.test, adds a whitespace after //, fixed the wording and adds tax calculation test for invoices.

The first change is a change because of a warning. We have no account there, so we can leave this blank as return_teams gets the current user in this case.

The second change is needed for the node_access check, as the format is required to do the check. The new patch also add this to the other content types. Which will decrease the test exceptions.

TODO:
- comments for tests
- working on the other test exceptions (maybe a second-patch?)

Status: Needs review » Needs work

The last submitted patch, ticketlist_edit_link-1448764-14.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, ticketlist_edit_link-1448764-14.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
133.78 KB

Oh found the error in the patch File.

Status: Needs review » Needs work

The last submitted patch, ticketlist_edit_link-1448764-14-fix.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
145.04 KB

New patch, which fixes the other warnings as well.

juliangb’s picture

I've read through the latest patch - although the comments are on style / text items, I've also checked the logic in the tests.

I haven't tested, but may not have time to do so for a few days at least. Happy with it based on my read through.

+++ b/stormexpense/stormexpense.test
@@ -62,5 +62,130 @@ class StormexpenseTestCase extends DrupalWebTestCase {
+    // Create stormperson with organization to userOrg

I'd suggest something similar to: "Create stormperson linked to userOrg in organization $org"

+++ b/stormexpense/stormexpense.test
@@ -62,5 +62,130 @@ class StormexpenseTestCase extends DrupalWebTestCase {
+    $this->assertRaw('node/'.$exp1->nid.'/edit', 'The Expense edit icon appears on the list');
+    $this->assertRaw('node/'.$exp1->nid.'/delete', 'The Expense edit icon appears on the list');

We should put spaces each side of the concatenation periods (applies elsewhere too). Sounds over the top, but this is now picked up by the branch style tests.

+++ b/storminvoice/storminvoice.test
@@ -55,6 +55,171 @@ class StorminvoiceTestCase extends DrupalWebTestCase {
+    //tax1: 20; tax2: 12; total: 132

Space between // and comment.

+++ b/stormnote/stormnote.test
@@ -61,4 +61,132 @@ class StormnoteTestCase extends DrupalWebTestCase {
+    $this->assertNoRaw('node/'.$note2->nid.'/edit', 'The Note edit icon does not appear on the list');
+    $this->assertNoRaw('node/'.$note2->nid.'/delete', 'The Note delete icon appears not on the list');
+

Delete text similar to edit text?

juliangb’s picture

I've attached a patch that makes the changes 2) 3) and 4) of the ones I suggested in #21. 1) is probably splitting hairs on thinking about it again.

juliangb’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev

Since my patch only changed style / wording rather than logic, I have pushed into 6.x-2.x.

juliangb’s picture

Status: Needs review » Needs work

The last submitted patch, ticketlist_edit_link-1448764-22.patch, failed testing.

juliangb’s picture

Status: Needs work » Fixed

Not needed in Project Management 7.x-1.x, as we're moving to using node grants.

Status: Fixed » Closed (fixed)

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