Defect #33415

Issue#closable? doesn't handle the case of issues with open subtask(s) ánd being blocked by other open issue(s)

Added by Mischa The Evil 4 months ago. Updated 4 months ago.

Status:NewStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:UI
Target version:-
Resolution: Affected version:

Description

Subject says it all. Given the current incarnation of Issue#closable?, the transition warning for an issue instance that is being blocked while simultaneously having (an) open subtask(s) does not include the 'being blocked' reason (as such including only the 'open subtask(s)' reason).

0001-POC-fix-for-33415.patch Magnifier (10.9 KB) Mischa The Evil, 2020-05-11 07:39

0001-Fix-33415-by-re-implementing-31589.patch Magnifier - Replaces 0001-POC-fix-for-33415.patch (11.3 KB) Mischa The Evil, 2020-05-16 07:53

0001-Fix-33415-by-re-implementing-31589-simplified.patch Magnifier - Replaces 0001-Fix-33415-by-re-implementing-31589.patch & 0001-POC-fix-for-33415.patch (7.98 KB) Mischa The Evil, 2020-05-19 06:10


Related issues

Related to Redmine - Feature #31589: Show warning and the reason when the issue cannot be clos... Closed

History

#1 Updated by Mischa The Evil 4 months ago

  • Related to Feature #31589: Show warning and the reason when the issue cannot be closed because of open subtasks or blocking open issue(s) added

#2 Updated by Mischa The Evil 4 months ago

Here's a preliminary POC fix against current master. Please review and comment.

#3 Updated by Mischa The Evil 4 months ago

  • Assignee set to Mischa The Evil

I'll post an improved and more cleaned-up iteration of this patch later today.

#4 Updated by Mischa The Evil 4 months ago

Here's the updated patch that replaces 0001-POC-fix-for-33415.patch (which I won't delete for patch evolution review purposes).
The patch is no longer a POC nor is it a final patch yet. There are still some small todo's left.

I ended up re-implementing #31589 in a slightly different manner to let Issue#closable? support the given case of issues with open subtask(s) while simultaneously being blocked by (an)other open issue(s).

Please review and let me know your comments.

#5 Updated by Marius BALTEANU 4 months ago

Mischa, the scope of this issue is to show in the UI all the reasons for why an issue cannot be closed, right?

#6 Updated by Mischa The Evil 4 months ago

  • Assignee set to Mischa The Evil

Marius BALTEANU wrote:

Mischa, the scope of this issue is to show in the UI all the reasons for why an issue cannot be closed, right?

Yes. At the moment, calling closable? on an issue that meets both the conditions (descendants.open.any? and blocked?) returns (somewhat) correctly, but doesn't populate the correct (n)or complete reason.
The cause of this is that Issue#closable? returns from the descendants.open.any? branch, as such not reaching the other if-branch. Likewise is support for a transition warning for an issue that meets both the conditions not implemented currently.

Marius BALTEANU wrote:

[...] show in the UI [...]

Not only. The CLI should also return the correct, precise reason.

Marius BALTEANU wrote:

[...] all the reasons [...]

All, except the workflow transition rules. As such can there be four transition warnings in total:
  • three transition warnings for Issue#closable?:
    1. descendants.open.any?
    2. blocked?
    3. descendants.open.any? && blocked?
  • one transition warning for Issue#reopenable?:
    1. ancestors.open(false).any?

I noticed that I have added more complexity than needed in the previous patches. I'll post an updated patch using a more simplified approach later today.

#7 Updated by Mischa The Evil 4 months ago

Here's the updated patch that replaces both 0001-Fix-33415-by-re-implementing-31589.patch and 0001-POC-fix-for-33415.patch (which I won't delete for patch evolution review purposes).
I don't consider the patch final yet. There are still some small todo's remaining, which I'd like to review and fix-if-needed first.

In this iteration I have removed the unnecessary complexity that I added previously to prevent unnecessarily calling methods multiple times. I simplified the taken approach to handle this, so this complexity wasn't needed any longer.

Some (additional) notes on the patch:
  • Code comments added to methods to clearly indicate what functionality they actually provide. A plugin developer looking at what's available for use might expect something other than what's covered by these methods. Maybe we should even shield them for internal use only?
  • Replaced short, single condition if structures to use a single-line if modifier.
  • For the two conditions: open_sub_tasks && !open_blocking_issues and open_blocking_issues && !open_sub_tasks, the negating part is currently optional. If those parts are omitted though, the order of checking for these conditions becomes leading for the result of this method. That might not be a big issue, but it could be broken in the future in case the order in the method is changed.
  • Return methods directly from within the if..elsif..else structures now we're using if..elsif structures to check for multiple conditions.

Please review and let me know your comments.

Also available in: Atom PDF