[16.0][MIG][REF] attachment_queue

[kevinkhao]

No description provided.

[kevinkhao]

@florian-dacosta might be of interest to you.

I did away with new environments and used savepoints. Unless I missed something, it doesn't make a difference.

[florian-dacosta]

Thanks for the work!

Another general change I would do : the menu to see the attachment queue has always been in a terrible place IMO, in configuration => technical => Database structure
Since it now depends on job, I would add a menu under the Job queue app.
Maybe on the same level of the queue menu.
(so make a menu, child of queue_job.menu_queue_job_root menu)

[florian-dacosta]

I am not sure, but I probably would put "attachment_queue" as for me it is more a technical name. Not sure if it is good to have upper case, spaces, etc...
I feel like I always have seen channel with a more technical name, so it would be at least for consistency. But I am not sure if it is a real issue or not.

[florian-dacosta]

I am not sure about this running_lock. It is set to True, but only for this transaction, it can't be read as True for a parallel transaction, since it is not commited.

What happens here, if the job run and then it is manually lauched, the job write this running_lock and go ahead.
The manual run then try to change the running_lock and fail due to a conccurent update error and then retry.
If the job is finished, then it will do nothing as the state won't be pending anymore.
So... It actually work but it is not perfect I guess because would work the same without this running_lock I guess (the concurrent update would be on writing the state for instance).
And the issue is that the user will have to wait for all this to happen...

Here is another proposal :
3 methods :

1. Run manually : can be done if state is pending or failed. No need to catch error, since it is synchronous, we raise the real error directly to the user and he can do what he has to do.

def run_manual(self):
try:
    self._cr.execute(f"""                                                           
        SELECT id
        FROM attachment_queue
        WHERE id  = %s
        FOR UPDATE NOWAIT""", self.id)
except psycopg2.OperationalError:
    raise UserError(the task is already running in a job)
if self.state != 'done'
    self.run()

2. run as a hob : the job should not fail and if the attachment processing fails, we put it to fail state and write the error (and eventually send the failure email)

def run_as_job(self):

try:
    self._cr.execute(f"""                                                           
        SELECT id
        FROM attachment_queue
        WHERE id  = %s
        FOR UPDATE NOWAIT""", self.id)
except psycopg2.OperationalError:
    raise RetryableJobError('Already runing, try again later', seconds=60, ignore_retry=True)
if self.state != 'pending':
  try:
      with savepoint():
          self.run()
      except Exception:
          self.write({"state": "failed", "state_message": str(e))
       emails = self.failure_emails
      if emails:
          send_mail()

3. common method to manual and job call, just run the attachment normally and write the state to done if no error was raised

def run(self)
  self._run()
  self.write({'state': 'done', 'date_done': now})

What do you think ?

[florian-dacosta]

Test should be fixed once #2625 is merged

[florian-dacosta]

@kevinkhao Green now !

[OCA-git-bot]

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). 🤖

[bealdav]

LGTM

[bguillot]

/ocabot merge nobump

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2560-by-bguillot-bump-nobump, awaiting test results.

[OCA-git-bot]

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