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.
Comment | File | Size | Author |
---|---|---|---|
#22 | ticketlist_edit_link-1448764-22.patch | 146.36 KB | juliangb |
#20 | ticketlist_edit_link-1448764-20.patch | 145.04 KB | kfritsche |
#18 | ticketlist_edit_link-1448764-14-fix.patch | 133.78 KB | kfritsche |
#14 | ticketlist_edit_link-1448764-14.patch | 133.78 KB | kfritsche |
#11 | ticketlist_edit_link-1448764-11.patch | 126.53 KB | kfritsche |
Comments
Comment #1
kfritschePatch 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?
Comment #2
kfritscheComment #3
juliangb CreditAttribution: juliangb commentedPerhaps 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.
Comment #4
kfritscheHere 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.
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?
Comment #5
juliangb CreditAttribution: juliangb commentedThe 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).
Comment #6
kfritscheYes this works and is much more clearer i think.
Comment #7
juliangb CreditAttribution: juliangb commentedI'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...
Is this related to the issue?
Typo?
Comment #8
kfritscheSecond 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
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)
Comment #9
kfritscheNew 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
Comment #10
juliangb CreditAttribution: juliangb commentedI 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?
Comment #11
kfritscheYou'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.
Comment #13
juliangb CreditAttribution: juliangb commentedThanks 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:
What's the context behind this change?
What's the context behind this change?
Comment #14
kfritscheNew 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?)
Comment #16
kfritsche#14: ticketlist_edit_link-1448764-14.patch queued for re-testing.
Comment #18
kfritscheOh found the error in the patch File.
Comment #20
kfritscheNew patch, which fixes the other warnings as well.
Comment #21
juliangb CreditAttribution: juliangb commentedI'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.
I'd suggest something similar to: "Create stormperson linked to userOrg in organization $org"
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.
Space between // and comment.
Delete text similar to edit text?
Comment #22
juliangb CreditAttribution: juliangb commentedI'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.
Comment #23
juliangb CreditAttribution: juliangb commentedSince my patch only changed style / wording rather than logic, I have pushed into 6.x-2.x.
Comment #24
juliangb CreditAttribution: juliangb commented#22: ticketlist_edit_link-1448764-22.patch queued for re-testing.
Comment #26
juliangb CreditAttribution: juliangb commentedNot needed in Project Management 7.x-1.x, as we're moving to using node grants.