Skip to content

Conversation

@simahawk
Copy link
Contributor

@simahawk simahawk commented Feb 8, 2021

No description provided.

@simahawk
Copy link
Contributor Author

simahawk commented Feb 9, 2021

@guewen I guess the test is wrong here because date_started in theory should be there always if you are calling set_done, right? It should call set_started before. See https://travis-ci.com/github/OCA/queue/jobs/481537133#L1082

@guewen
Copy link
Member

guewen commented Feb 9, 2021

I guess the test is wrong here because date_started in theory should be there always if you are calling set_done, right? It should call set_started before

It's not wrong, for this test, the initial state started had no impact on set_done. It's a unit test, it doesn't test the full flow.
You added a new dependency (date_started) in this unit, so you have to be sure the initial state of the test set it correctly.
You shouldn't call set_started before, because then the test would test set_started and set_done.
You only need to set job_a.date_started = xxx as this is the initial state you miss.

@simahawk
Copy link
Contributor Author

simahawk commented Feb 9, 2021

better?

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks!! ❤️

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, approving

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

@simahawk simahawk force-pushed the 13-add-exec_time branch 2 times, most recently from b84fc86 to 14862e2 Compare February 10, 2021 09:35
<field name="exec_time" type="measure" />
</graph>
</field>
</record>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a pivot view gives better information:

<pivot string="Queue Job">
    <field name="exec_time" type="measure"/>
    <field name="job_function_id" type="row"/>
    <field name="date_done" type="col" interval="week"/>
</pivot>

Example:
Example of pivot view

Note: in this example, I used group_operator="avg" on the field, which I'm not sure we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The average is really cool on this view as we can see if it grows over time...

Copy link
Contributor Author

@simahawk simahawk Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. At least is a great start!

@guewen
Copy link
Member

guewen commented Feb 10, 2021

Pushed a fixup for pivot view + group by avg by default. I added a mention in string and help for the avg to prevent confusion.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@guewen
Copy link
Member

guewen commented Mar 4, 2021

Hi @simahawk :)

I squashed my commit and rebased, can I remove the work in progress label? I'd like to merge if the build is green.

@guewen
Copy link
Member

guewen commented Mar 4, 2021

I squashed my commit and rebased, can I remove the work in progress label? I'd like to merge if the build is green.

Oh no sorry, I couldn't push it because I do no longer have push writes here 😆
Could you do it?

@simahawk
Copy link
Contributor Author

simahawk commented Mar 4, 2021

welcome back! :)
It's done!

@guewen
Copy link
Member

guewen commented Mar 4, 2021

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-309-by-guewen-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4e7d0ef into OCA:13.0 Mar 4, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 9bf3f24. Thanks a lot for contributing to OCA. ❤️

guewen pushed a commit to guewen/queue that referenced this pull request Oct 24, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
nguyenminhchien pushed a commit to nguyenminhchien/queue that referenced this pull request Nov 25, 2023
yibudak pushed a commit to yibudak/queue that referenced this pull request Nov 27, 2023
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 19, 2024
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 24, 2024
thienvh332 pushed a commit to thienvh332/queue that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants