-
Notifications
You must be signed in to change notification settings - Fork 11
[WIP] Make checking optional for dependent encoding profiles #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7d93320 to
b5eacb9
Compare
| SET ROLE TO postgres; | ||
|
|
||
| CREATE OR REPLACE FUNCTION ticket_state_next(param_project_id bigint, param_ticket_type enum_ticket_type, param_ticket_state enum_ticket_state) | ||
| CREATE OR REPLACE FUNCTION ticket_state_next(param_ticket_id bigint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jjeising @pegro
Continuing discussion
This doesn't work, Ticket::expandRecording calls Ticket::queryPreviousState with state='preparing', differing from the current state of the ticket, which would usually be cutting.
I'd suggest using an additional parameter skip_dependent DEFAULT FALSE and adding a new function (maybe with a trigger as suggested) ticket_is_master to pass that in from outside and keep this function completely ticket agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before, if determining a next/previous ticket state depends on more as just the ticket type and state, I'm fine with changing all functions to just requiring a ticket_id and migrate all users accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with changing all functions to just requiring a ticket_id
The new issue here is: ticket_state_next is also used with a state different from the current state of the ticket (with param_ticket_id).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to query for all possible ticket states of the current ticket, I would then add a second optional parameter param_state as a state of reference which defaults to the current ticket state.
So you could call ticket_state_next(param_ticket_id) to get the next state in relation to the current state. And you could call ticket_state_next(param_ticket_id, param_ticket_state) to query for the next state the ticket would get advanced to, if the ticket would be in state param_ticket_state.
At least that's what I'd suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zuntrax That's what we discussed previously, can you implement that?
|
|
||
| public static function isSkippable($state) { | ||
| return ($state === 'postencoded' || | ||
| $state === 'checking'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh I don't really like hard-coding stuff like that here.
Do we usually allow this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved to tbl_state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean tbl_ticket_state, but yes.
Another question: should this setting only be visible if any profiles with a dependency are assigned to the project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question: should this setting only be visible if any profiles with a dependency are assigned to the project?
I'm not sure if it is worth introducing additional complexity in the view. It's only two checkboxes at the moment, which have a default value.
|
|
||
| WHILE ret IS NOT NULL LOOP | ||
| SELECT * INTO next_state FROM ticket_state_next(param_project_id, param_ticket_type, ret); | ||
| SELECT * INTO next_state FROM ticket_state_next(param_ticket_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: eliminating ret from parameters will probably produce endless loops.
|
Superseded by #217. |
Closes #160
Resubmitted from #162