iTop
iTop copied to clipboard
Combodo
Add "filter list" menu item for any datatable widget
There was already two existing actions :
- Add To Dashboard
- Create a Shortcut
This adds a new icon "Filter list". This options is present in any datatable, except when we are in a search. That means this icon will appear in :
- list dashlet
- 1-n or n-n attributes
Screenshot in the Team object's members tab :
TODO :
- [x] MUST : Rename dict key to match option label (UI:Menu:OpenSearch => UI:Menu:FilterList)
- [x] MUST : Add menu entry as a permanent icon instead of a menu entry (keeping the exclusion : this should not be displayed in the search page itself !). Use the magnifier icon (already present for the same thing in object details)
- [ ] MUST : can we rebuild our own filter when generating the icon ?
- [ ] WANT : when opening Person.tickets_list, the Person filter is set on the joined class (Person.id) instead of root class (Ticket.caller_id). This causes the filter criteria to be unreadable. See why this query is used, and if we can change it so that the query will be easier to understand and modify in the search page ? => see https://github.com/Combodo/iTop/pull/222#issuecomment-967248508
- [ ] WANT : for n-n links, we are currently having SELECT lnk, remote. It is nice to have both classes as we could then add criteria on both ! But for the search we should open SELECT remote, lnk instead. Having this modified query will allow to have a clearer search title. => see https://github.com/Combodo/iTop/pull/222#issuecomment-967256547
- [ ] WISH : n-n lnk with attributes, hide the lnk external key to the remote
- [ ] WISH : n-n lnk with only external keys, only display remote class fields
Yes it's really dirty :( We'll have a chat with the rest of the dev team, I might have missed something obvious. At least we could add some ContextTag...
When making a PR formatting all the files makes it very difficult to see what you actually changed 😕
It's not all the files and not even the complete file, but it's true, I had a hard time to review the actual changes as well..
Well you know what I meant, in this file there are tones of changes that you have to check to find that only the 4 last lines have been modified. 🤯 (And I didn't capture the rest of the file!)
I see that the same URL already is generated and is available as $sURL:
https://github.com/Combodo/iTop/blob/85729d2ea88204faa01d431078c16b70e4061a67/application/utils.inc.php#L1389-L1390
Whereas for the feature itself, @v-dumas might be interested by it.
Yes indeed, this is excellent if we could have it in LinkedSet and DashletList and if it can prefilter the search automatically with the appropriate criteria that would be perfect. Then, assuming that action has an id, it could be displayed outside of the menu drop-down just by its icon, the day iTop knows how to do that, your challenge @steffunky !
About formatting, well, this is not easy for a file like this, as I needed to add lines in a non well formatted switch case... I could have done better maybe (disabling the save action plugin, change the lines, commit, re-enable the plugin), but when you're in middle of your dev it's not that easy to forsee this... I will push a commit reverting those comsetic changes, that will make the diff easier to read in GitHub.
I really advise to review code inside PHPStorm : PR are easy to browse and open, diffs can be seen without even doing a branch checkout, and indentations changes (actually white spaces) can be removed from display.
I had a quick chat with v-dumas : a possible Combodo's internal ref is N°3200. We will discuss face to face next week on this subject :)
Dev team reviewed this PR. Conclusion :
- [x] no emoji in the menu items as the rendering will be platform dependent, we should rather use Font Awesome icons, and add icons also to existing keys (dashlet, shortcut) (see Hyperlink configurator extension)
- [x] the code that state if we are already in a search should be extracted in its own method
- [x] rather than testing on the current URL if we're in a search screen, use a aExtraDataParam if possible, or of not create new ContextTag ?
- [x] add phpdoc for new methods :)
I see that the same URL already is generated and is available as
$sURL:
I'll have a look, thanks !
About formatting, well, this is not easy for a file like this, as I needed to add lines in a non well formatted switch case... I could have done better maybe (disabling the save action plugin, change the lines, commit, re-enable the plugin), but when you're in middle of your dev it's not that easy to forsee this... I will push a commit reverting those comsetic changes, that will make the diff easier to read in GitHub.
I really advise to review code inside PHPStorm : PR are easy to browse and open, diffs can be seen without even doing a branch checkout, and indentations changes (actually white spaces) can be removed from display.
No need to push another commit for the formatting (IMHO) as we already reviewed the changes. It was more a thought for next PRs.
Reviewed this today with @v-dumas.
Conclusions :
- [x] remove emoji indeed, but don't use FA icons yet (this will be subject of another FAF maybe, there are multiples issues around this, one is the icon that needs to be kept when transforming a menu item to a button using the
shortcut_actionsconfig parameter - N°4078 in Combodo internal DB) - [x] change label to "Filter list..."
- [x] see what's happening when adding this menu item id in the
shortcut_actionsconfig parameter ? - [x] in list dashlets we have a "menu" option that adds some icons (new object, refresh). Try to see if we could add the filter icon ?
- 🤔 in iPopupMenuExtension::EnumItems we have a $iMenuId parameter that gives where the menu is located, that could be an hint to add the filter option only on screens that are not the search itself
Test adding the new menu item in the shortcut_actions config parameter. But this isn't working :/
Rendering is done in \MenuBlock::GetRenderContent, which is called twice. First time the item is not present, and on the second time it is but in the $aToolkitActions variables. The config param only has effect on the items present in the $aRegularActions or $aTransitionActions variables...
We may create a ticket to fix that in the future...
Adding the menu item in dashlet menu : this would be a separate dev, I prefer not to do it now. The job seems to be done in \DisplayBlock::RenderList (search for $bDisplayMenu var)
=> PR ready for new review
We reviewed it with @piRGoif yesterday:
- icon used could well be the already used magnifier as it does the same opening of a class search
- the menu should always be there on any list, as a simple icon always displayed
- when we open in a search, a many to many relationship, then the search executed should be a SELECT R,L FROM RemoteClass AS R JOIN LinkClass AS L ON ... thus it allow the user to filter both on the remote class and the link attributes if they are some. Bonus, if they aren't any attribute (apart from the 2 ids, magic attributes and ExternalFields) in the LinkedClass, we could limit the search to the remote class.
- when opening a n:1 relationship such as the Ticket of a Person, we have noticed that due to the way the query was build, the preset criteria is not easy to read and cannot be changed. Instead of SELECT Ticket AS T WHERE T.caller_id='xx', it does SELECT Ticket AS T JOIN Person AS P ON T.caller_id=P.id WHERE P.id='xx'. Perform those 2 queries in a Run Query, save as Shortcut and open the Shortcut, you will see the difference in terms of preset criteria in the search.
- the menu should always be there on any list, as a simple icon always displayed
When you say this, you mean always but not in a search right? Otherwise it will be confusing.
- the menu should always be there on any list, as a simple icon always displayed
When you say this, you mean always but not in a search right? Otherwise it will be confusing.
Yes this is the idea : instead of adding it everywhere but in the search page as a menu entry, add it as a permanent icon like create and refresh.
Person.tickets_list
For now we are generating this query :
SELECT `Ticket` FROM Ticket AS `Ticket` JOIN Person AS `Person` ON `Ticket`.caller_id = `Person`.id WHERE (`Person`.`id` = :id)
instead of :
SELECT `Ticket` FROM Ticket AS `Ticket` JOIN Person AS `Person` ON `Ticket`.caller_id = `Person`.id WHERE (`Ticket`.`caller_id` = :id)
The method doing this is \AttributeLinkedSet::GetDefaultValue It is :
- creating a $oMyselfSearch DBObjectSearch on the current object class with its id
- creation a $oLinkSearch DBObjectSearch on the remote object
- doing
$oLinkSearch->AddCondition_PointingTo($oMyselfSearch, $sExtKeyToMe);
I've asked the dev team if this could be changed...
n-n attributes : same as before, this is done using the same means in \AttributeLinkedSet::GetDefaultValue (no impl in the child class AttributeLinkedSetIndirect, but in this impl we have a test : if ($this->IsIndirect())), and we are also calling \DBObjectSearch::AddCondition_PointingTo.
Discussed with Romain about the DBSearch generation in \AttributeLinkedSet::GetDefaultValue :
- this was like this from the very start (see b756db09 in... 2009 !!!)
- browsing the code history, we didn't find any specific reason for why the condition is set on Person and not Ticket ?
There are two choices :
- either changing the code in \AttributeLinkedSet::GetDefaultValue
- or generating our own filter when creating the "filter list" icon ?
I'll have to check if the later is possible (do we have all the context we need when we generate the icon ? )