-
Notifications
You must be signed in to change notification settings - Fork 142
[IMP] runbot: add kanban view and stages to build error page #1071
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
base: 18.0
Are you sure you want to change the base?
Conversation
runbot/models/build_error.py
Outdated
description = fields.Text(string='Stage description', translate=True) | ||
sequence = fields.Integer('Sequence', default=1) | ||
fold = fields.Boolean(string='Folded in Kanban', default=False) | ||
|
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 it is to work, we will also need to change values of "active" and "tes_tags" every time stage changes to either solved/unsolved/ignored. And vice versa, if user decides to change te value of "active" or "test_tags" fields, we stage should also change based on that.
Otherwise, we will have to manually change the stage of every already solved ticket from unsolved to done.
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 for some like "solved" field we should in theory remove them as they are redundant with the stages. But as a lot of other code depends on it I guess I will need to make it the same as the existing field and have a script to run at install to adapt records.
But you are right I should be more aware of this, I will rethink about it when the stages will be totally decided. For instance should issues that did not happen since X days be moved to "ignored" automatically ?
86aacd9
to
43aa6bd
Compare
while looking if we can remove the "Open (not fixed)" field, I realized that it is technically the |
43aa6bd
to
713ff65
Compare
@angv-odoo would you mind giving it another look please ? :) |
TODO: stage ignored for test-tags (constraint or put it in "ignored" stage & in migration) |
713ff65
to
9108aa8
Compare
@angv-odoo ready fora full review if you have some time :) |
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.
Personally I am not convinced by the idea of a kanban view for this model, but to each their own, I wouldn't make it the default, that's for sure.
Thanks for your work though :)
runbot/models/build_error.py
Outdated
name = fields.Char(string='Stage Name', required=True) | ||
description = fields.Text(string='Stage description') | ||
sequence = fields.Integer('Sequence', default=1) | ||
fold = fields.Boolean(string='Folded in Kanban', default=False) |
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 don't think any of those fields are actually relevant and stage_id
could be a selection field IMO
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.
it can indeed be and I started it as a selection field.
But it does have its shares of drawback:
- No way to fold the stages by default (at least, to my knowledge) -> in standard it looks like there is the need for the
fold
record to have this feature - No way to dynamically add new stages (but this can indeed be argued as it will likely not be the case and can be added in the code)
- There was some issue to show all stage; some would dissapear if there was no record with this stage (but it might be possible standard-ly with
group_expand=True
, but I'm pretty sure it didn't worked when I tried out before)
Let me know your opinion if this drawbacks are not relevant in your opinion and I will switch it as a selection field
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.
You can do that with a custom group_expand
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 did the changes for a selection field. The folding part can only be done through overidding _read_group_fill_results
if I'm not mistaken (at least on the python side
runbot/models/build_error.py
Outdated
self.search([ | ||
('stage_id', '=', build_error_stage_done), | ||
('last_seen_date', '<', now - relativedelta(days=nbr_day_done_to_archive)), | ||
]).write({'active': False}) |
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.
We don't want to have a fully automated way to archive error for now. We already thought about doing something like tracking all forwardport pr and detecting that all fixing pr are merged and that the error did not appear since it was merged, but this is really arbitrary to mark as solved if it diddn't appear for a while
- error that breaks every year, and is not fixed yet.
- error that changed of error message because of another refactoring (need to be cleaned, merged)
Keeping it alive will allow to easily identify the cause and potential existing conversations. With this logic it would be archived.
If you don't want to have old error as noise in your view, you can just add a filter to remove error not seen for a while. There is actually one for the opposite (disaplay olfd errors only) that we use sometime to do this operation manually, but checking first it it looks legit. (merged fixing pr, very rare, not appearing anymore and no similar error found)
We are working to reduce the number of error using qualifier and adding to to help identify if the error really disappeared, this will help to do something similar in the future but much more precise.
Also, we NEVER want to disable an error with a test-tag without being aware of that, it must be a manual operation followed if particular attention on the following stagings. An error may disappear just because the error message changed slightly.
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 will remove the archive part. The "Done" stage is enough in my opinion so that we know that the issue is likely solved. As the done stage is folded by default in the kanban view it will not bother the user. We will let you the decision of archiving them like it is currently
9deba33
to
a364997
Compare
@Williambraecky could you re-review when you have some time please ? :) |
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.
Hello 👋
We are fine with the kanban view, however:
- We do not want any automation without really thinking about it and agreeing together about what should do what, if you wish to discuss it further you are welcome to come to our office or start a thread/task about it :)
- We do not want to make the kanban view the default view, we are very used to working with the list view and having to handle state is not something we want as of now, at least not more than what we currently have.
The comments below have been made with that in mind, if you still with to have the kanban view without the automation those changes should be made/considered and we can merge the view by itself :)
Cheers,
runbot/views/build_error_views.xml
Outdated
@@ -478,7 +534,7 @@ | |||
<field name="name">Errors</field> | |||
<field name="res_model">runbot.build.error</field> | |||
<field name="path">error</field> | |||
<field name="view_mode">list,form</field> | |||
<field name="view_mode">kanban,list,form</field> |
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.
<field name="view_mode">kanban,list,form</field> | |
<field name="view_mode">list,kanban,form</field> |
runbot/__manifest__.py
Outdated
@@ -18,6 +18,7 @@ | |||
'data/runbot_data.xml', | |||
'data/runbot_error_regex_data.xml', | |||
'data/website_data.xml', | |||
'data/ir_cron_data.xml', |
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.
Remove this
runbot/data/ir_cron_data.xml
Outdated
@@ -0,0 +1,10 @@ | |||
<odoo> |
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.
Remove the file
runbot/models/build_error.py
Outdated
help="New: Error is new and not yet solved. Build error not happening for a long time in this state will automatically move to Done\n" | ||
"Solved: Error should be solved. Build error not happening for a small time in this state will automatically move to Done\n" | ||
"Ignored: Error is ignored. No change of state will apply\n" | ||
"Done: Error is done. No change of state will apply except if issue re-appear, then it will go back to New" |
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.
Adapt the help message, no automation as of now.
runbot/models/build_error.py
Outdated
state = fields.Selection([ | ||
('new', 'New/Unsolved'), | ||
('solved', 'Solved'), | ||
('ignored', 'Ignored'), | ||
('done', 'Done'), |
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.
Since we aren't doing automation I don't think the done stage is really necessary, once an error is solved it is archived to the done column would only contain archived errors anyways.
runbot/models/build_error.py
Outdated
def _update_state(self, nbr_day_solved_to_done=7, nbr_day_new_to_done=15): | ||
"""Called automatically by scheduled action to update the state of the error if necessary""" | ||
now = fields.Datetime.now() | ||
|
||
# Done errors that did happen again recently are moved back to new | ||
self.search([ | ||
('state', '=', 'done'), | ||
('last_seen_date', '>=', now - relativedelta(days=nbr_day_new_to_done)), | ||
]).write({'state': 'new'}) | ||
|
||
# New error that did not appear after a long time are marked as done | ||
# Solved error that did not appear after a short time are marked as done | ||
self.search([ | ||
'|', | ||
'&', | ||
('state', '=', 'new'), | ||
('last_seen_date', '<', now - relativedelta(days=nbr_day_new_to_done)), | ||
'&', | ||
('state', '=', 'solved'), | ||
('last_seen_date', '<', now - relativedelta(days=nbr_day_solved_to_done)), | ||
]).write({'state': 'done'}) |
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.
Remove this
runbot/views/build_error_views.xml
Outdated
<widget name="web_ribbon" title="Test-tags" bg_color="bg-danger" invisible="not test_tags"/> | ||
<field name="name" class="fw-bold fs-5"/> | ||
<group> | ||
<div style="display: flex; align-items: center;"> |
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.
<div style="display: flex; align-items: center;"> | |
<div class="d-flex align-items-center"> |
env = api.Environment(cr, SUPERUSER_ID, {}) | ||
|
||
# Archived build errors are set are considered Done | ||
env['runbot.build.error'].search([['active', "=", False]]).write({'state': 'done'}) |
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 we remove done
env['runbot.build.error'].search([['active', "=", False]]).write({'state': 'done'}) | |
env['runbot.build.error'].search([['active', "=", False]]).write({'state': 'solved'}) |
env['runbot.build.error'].search([['active', "=", False]]).write({'state': 'done'}) | ||
# Build errors with test_tags are set to Ignored | ||
env['runbot.build.error'].search([['test_tags', '!=', False]]).write({'state': 'ignored'}) |
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 know those are very simple operations but they should honestly be done through raw sql queries.
f42868e
to
6abb64f
Compare
@Williambraecky thanks for the detailed review :) I think I got carried away by the original PR idea. Let's start gentle and we will see with time if it needs automation or not you are right. |
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.
Looks good to me 👍
Thanks :)
NOTE: I haven't pulled the latest version to test but code seems quite similar so I didn't deem it necessary.
cr.execute(""" | ||
UPDATE runbot_build_error | ||
SET state = 'ignored' | ||
WHERE test_tags is not null |
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.
To be more precise:
WHERE test_tags is not null | |
WHERE test_tags is not null AND active IS TRUE |
e7743bd
to
f4aa646
Compare
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.
Tested locally and found some issues :)
runbot/views/build_error_views.xml
Outdated
<div class="d-flex align-items-center gap-1"> | ||
<i class="fa fa-code-fork" title="Concerned Odoo versions"/> | ||
<field name="version_ids" widget="many2many_tags"/> | ||
</div> |
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.
This field is causing a lot of latency as it is not stored and is a related field.
For now let's remove it :)
<div class="d-flex align-items-center gap-1"> | |
<i class="fa fa-code-fork" title="Concerned Odoo versions"/> | |
<field name="version_ids" widget="many2many_tags"/> | |
</div> |
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.
should I do the same for trigger_ids
as it's also computed not-store ?
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.
trigger_ids
is fine, as it it computed stored, I tested locally with the diff above and perf was good 👌
# Build errors with test_tags are considered ignored | ||
cr.execute(""" | ||
UPDATE runbot_build_error | ||
SET state = 'ignored' |
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.
ignored
does not seem to be an appropriate name for errors that should be considered as a priority to fix.
Why not disabled
or neutralized
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.
no one told me there was a priority to fix, it does indeed make sense in this case to show them and adapt the state name. Thanks for the info :)
runbot/models/build_error.py
Outdated
@@ -586,6 +595,15 @@ def action_link_errors(self): | |||
base_error = self_sorted[0] | |||
base_error._merge(self_sorted - base_error) | |||
|
|||
@api.model | |||
def _read_group_fill_results(self, domain, groupby, annoted_aggregates, read_group_result, read_group_order=None): | |||
# Override to fold ignored 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.
Why should we fold errors that have to be fixed in priority ?
5a22646
to
fddd35b
Compare
Introduce a kanban view to the runbot build error records. Kanban cards show the most valuable field with relevant icons. The concept of "state" was also introduced for the build error model. Currently there is 3 states: 1. New: default state for any new error build 2. Solved: when the issue should be solved 3. Disabled: build error currently disabled (test-tags) States are expected to be used by the user. For instance after fixing an underteministic error the user can change the state from new -> solved so that the user knows that he's actively not working on it
fddd35b
to
d846b4e
Compare
Introduce a kanban view to the runbot build error records.
Kanban cards show the most valuable field with relevant icons.
The concept of "state" was also introduced for the build error
model.
Currently there is 3 states:
States are expected to be used by the user. For instance after fixing an underteministic error the user can change the
state from new -> solved so that the user knows that he's actively not working on it
Visuals (Note: the state "Done" have been removed):

