Allow state skipping for dependent encoding profiles #217
Conversation
Next, previous and commence states are now only dependent on the ticket. All other required parameters are inferred.
…xt() Add boolean skippable_on_dependent to tbl_ticket_state Add boolean skip_on_dependent to tbl_project_ticket_state Change signature of ticket_state_next. Now it takes only the ticket_id as parameter and returns the next state of that ticket based on the current state. Optionally it takes a ticket_state as second parameter and returns the next state based on the assumption the ticket was in that state.
| JOIN tbl_encoding_profile_version epv ON epv.id = t.encoding_profile_version_id | ||
| JOIN tbl_encoding_profile ep ON ep.id = epv.encoding_profile_id | ||
| WHERE t.id = param_ticket_id | ||
| ) IS NULL ) |
There was a problem hiding this comment.
Second time the exact same subquery is used. Would it be worth it to have is is_master_ticket(ticket_id)?
| FROM | ||
| tbl_ticket t | ||
| WHERE | ||
| t.id = param_ticket_id; |
There was a problem hiding this comment.
What happens if that ticket_id is not found?
There was a problem hiding this comment.
Then ticket_state_initial(NULL, NULL) is called which results in
which returns NULL, so the while loop is not entered and function returns NULL.
There was a problem hiding this comment.
So, nothing breaks but the warning message is misleading. Mh.
Maybe adding a IF NOT FOUND THEN clause might be worth it.
| ADD COLUMN skip_on_dependent boolean NOT NULL DEFAULT false; | ||
|
|
||
| ALTER TABLE tbl_ticket_state | ||
| ADD COLUMN skippable_on_dependent boolean NOT NULL DEFAULT false; |
There was a problem hiding this comment.
That column only exists in the migration patch?!
There was a problem hiding this comment.
And it only seems to be used when displaying the states, but a database user would be allowed to set that flag on states not marked as skippable? or did I miss something
There was a problem hiding this comment.
And it only seems to be used when displaying the states, but a database user would be allowed to set that flag on states not marked as skippable?
It belongs to the state, so it's stored together with states. I'm not sure we need additional validation if the state marked to skip in a project is skippable?
There was a problem hiding this comment.
That column only exists in the migration patch?!
Fixed, thanks.
| FROM tbl_ticket t2 | ||
| WHERE t2.project_id = param_project_id AND param_project_id IS NOT NULL | ||
| ) AS x | ||
| WHERE t.id = x.id; |
There was a problem hiding this comment.
Not sure why that needs a subquery?!
There was a problem hiding this comment.
The function "ticket_state_next" shall not be called twice per row. No idea how to use the result of a function call in a SET block of an UPDATE statement, since you need the state and its executable flag separately.
| echo $f->checkbox('States[' . $index . '][service_executable]', null, $state['project_service_executable'], [], false); | ||
| } ?></td> | ||
| <?php if (!empty($skip)): ?> | ||
| <td class="right"><?php if ($state['skippable_on_dependent']) { |
There was a problem hiding this comment.
Regarding the missing validation: if the state is not marked as skippable, you won't be able to see whether flag is set on the state or not. So yeah, via UI you won't be able to create a configuration in which a state not marked as skippable is skipped. But if someone or something messes up your database, you wouldn't be able to tell, why states are skipped!?
So maybe show the checkbox if either skippable OR skip is set. And add a validation? Or show a disabled checkbox?
Implements #160, supersedes #171.
We may additionally want to check if the ticket is currently in a skippable state and do not skip states in this case. This would happen if the user manually set the ticket to a skippable state (e.g. ticket is currently in checking).
@pegro Requested changes from #171 should be implemented, can you have a look?