Refine request tabs visibility conditions #14347
Conversation
ncounter
force-pushed
the
refine-request-tabs-visibility-conditions
branch
from
8158e08
to
3934053
Compare
ncounter
force-pushed
the
refine-request-tabs-visibility-conditions
branch
from
3934053
to
116af26
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #14347 +/- ##
==========================================
- Coverage 87.90% 87.90% -0.01%
==========================================
Files 736 736
Lines 24753 24759 +6
==========================================
+ Hits 21759 21764 +5
- Misses 2994 2995 +1 |
| @@ -199,4 +199,18 @@ def next_prev_path(**opts) | |||
| request_show_path(parameters) | |||
| end | |||
| end | |||
|
|
|||
| # Handle tabs visibility | |||
| def request_action_tab_visibility(action, tab_name) | |||
There was a problem hiding this comment.
Instead of extracting the data from the action and then computing the logic, we should ask the action if I should render that tab or not. So ideally each bs request action class should have a method telling the view if it should render the tab or not.
There was a problem hiding this comment.
I see your point but, to be honest, I prefer to avoid view conditions in the model. It if was about a policy or specific property of the action itself I would agree 100%. In this case instead, the model is ignorant about how the elements are rendered and presented, it is just a view condition method that belongs to the view, and I just wrapped and centralized in a method to be more handy. If the webui page will change in the future (like no tabs, or anything different), the model should be loose from that change.
There was a problem hiding this comment.
I'm okay in avoiding view conditions in the model, but I think we can live with that in order to achieve better code structure. Also the view helpers are tightly coupled to the bs_request_action details right now, and I think that's worse
And the view knows too much: we're breaking the Tell, don't ask principle.
If you are not comfortable with that, then maybe we need a decorator, a class sitting between the model and the view that encapsulates the "view logic" (as we do with Finders).
|
Superseded by #14349 |
Request action tabs visibility are too generic. In case of a
:deleteaction, the following conditions are missing:changestab should be visible only if the:deleteaction's target is apackage. If it is aprojectnochangestab insteadmentioned issuestab should never be visible in case of a:deleteactionThis PR, based on #14346, refine the visibility conditions according to the above statements.
Reviewers
Do not consider the first three commits, they are included in #14346 instead. This PR is rebased on top of that one