[dev.icinga.com #9220] Centralize submission and apply handling of sort rules

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/9220

Created by jmeyer on 2015-05-04 13:09:56 +00:00

Assignee: aklimov
Status: Resolved (closed on 2015-05-12 08:35:03 +00:00)
Target Version: 2.0.0-rc1
Last Update: 2015-05-12 08:35:03 +00:00 (in Redmine)

---

Our SortBox submits its changes using POST, but does not process such by itself. Instead the monitoring module's ListController does this using hasBetterUrl(), which cannot be re-used in other controllers. To solve this, it is required to implement the redirect handing in the frameworks base controller (Web\Controller). It is crucial to process such a request as early as possible, so I suggest to use init() for this. Once this is done, please remove hasBetterUrl() and all of its usages.

Additionally, there is no centralized handling how to apply submitted sort rules on a view's query. This is currently done by the monitoring module's ListController using filterQuery(). To centralize this (fetching sort and dir from the request and calling order() on a query) it is required to teach the SortBox to handle this case. SortBox::setQuery(Sortable $query) fits this purpose very well. Applying the parameters should be done by SortBox::handleRequest(). (Please also rename applyRequest() to setRequest()) The frameworks base controller's setupSortControl helper method needs to be adjusted as well so that it accepts the query as second optional parameter and initiates then the request processing by calling setRequest, setQuery and handleRequest. Again, once this change has been done, please remove the processing of the sort and dir parameters in ListController::filterQuery().

Changesets

2015-05-11 11:36:58 +00:00 by aklimov 152c6a8

Merge Monitoring_ListController::hasBetterUrl() into Icinga\Web\Controller::init()

refs #9220

2015-05-11 12:07:57 +00:00 by aklimov 295254d

Rename Icinga\Web\Widget\SortBox::applyRequest() to ...::setRequest()

refs #9220

2015-05-11 13:37:00 +00:00 by aklimov 17ebe07

Implement Icinga\Web\Widget\SortBox::setQuery()

refs #9220

2015-05-11 14:09:20 +00:00 by aklimov 3265964

Implement Icinga\Web\Widget\SortBox::handleRequest()

refs #9220

2015-05-11 14:28:23 +00:00 by aklimov e1c3d23

Icinga\Web\Controller::setupSortControl(): set query on the newly created SortBox (if given)

refs #9220

2015-05-12 08:17:38 +00:00 by aklimov dd58f14

Don't apply sort rules in Monitoring_ListController::filterQuery()

refs #9220

2015-05-12 08:29:50 +00:00 by aklimov afa0dc0

SortBox::handleRequest(): check whether $this->query !== null

refs #9220

2015-05-12 08:32:24 +00:00 by aklimov 4bda1cf

Merge branch 'bugfix/Centralize-submission-and-apply-handling-of-sort-rules-9220'

fixes #9220

---

Relations:

• relates #8339

[icinga-migration]

Updated by tgelf on 2015-05-04 22:38:15 +00:00

I'm pretty sure you'll reflect your wording as soon as you switched this to GET. Good luck.

The way to get rid of this method this is teaching "sort" and similar controls to handle their own requests. As we have wonderful forms right now this should be very easy to implement. Unfortunately there wasn't such at the time being. "Controller" is IMO one of the worst places for this code. And your example is mixing responsabilities from completely different domains, it would lead to even more redundant code throughout our controllers.

Cheers,
Thomas

[icinga-migration]

Updated by jmeyer on 2015-05-05 06:09:08 +00:00

tgelf wrote:

I'm pretty sure you'll reflect your wording as soon as you switched this to GET. Good luck.

The way to get rid of this method this is teaching "sort" and similar controls to handle their own requests. As we have wonderful forms right now this should be very easy to implement. Unfortunately there wasn't such at the time being.

You're absolutely right, as I was already expecting such a response I wrote the "for god's sake" part, but did not calm down myself. Sorry for that. :)

tgelf wrote:

"Controller" is IMO one of the worst places for this code. And your example is mixing responsabilities from completely different domains, it would lead to even more redundant code throughout our controllers.

Again. You're right, SimpleQuery::order or preferable a helper method would be a better alternative. (I'm speaking about accessing GET parameters, of course, like SimpleQuery::paginate does ;) ) But how this needs to be implemented is exactly what my example shows, part 2 and 3 are already done by SimpleQuery::order though.

[icinga-migration]

Updated by jmeyer on 2015-05-05 13:57:54 +00:00

Johannes wrote:

tgelf wrote:
> "Controller" is IMO one of the worst places for this code. And your example is mixing responsabilities from completely different domains, it would lead to even more redundant code throughout our controllers.

Again. You're right, SimpleQuery::order or preferable a helper method would be a better alternative. (I'm speaking about accessing GET parameters, of course, like SimpleQuery::paginate does ;) ) But how this needs to be implemented is exactly what my example shows, part 2 and 3 are already done by SimpleQuery::order though.

Nevermind. Seems like that there is still something to discuss: #8339

[icinga-migration]

Updated by jmeyer on 2015-05-05 13:58:02 +00:00

• Relates set to 8339

[icinga-migration]

Updated by jmeyer on 2015-05-11 09:22:10 +00:00

• Subject changed from Submit SortBox changes using GET to Centralize submission and apply handling of sort rules
• Description updated
• Status changed from New to Assigned
• Assigned to set to aklimov

[icinga-migration]

Updated by aklimov on 2015-05-12 08:19:39 +00:00

• Status changed from Assigned to Feedback
• Assigned to changed from aklimov to jmeyer

[icinga-migration]

Updated by jmeyer on 2015-05-12 08:26:04 +00:00

• Status changed from Feedback to Assigned
• Assigned to changed from jmeyer to aklimov

[icinga-migration]

Updated by aklimov on 2015-05-12 08:35:03 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 0 to 100

Applied in changeset 4bda1cf.