[FIX] web_editor, website: prevent deletion of used attachment

[bso-odoo]

The deletion of used attachments from the media dialog is prevented
since 1. It is possible that it worked at the time, but that it
stopped working in 2 when the routes for attachments have been
updated.
Also, it only checked for the presence of attachments in views,
ignoring other HTML fields.

This commit fixes the existing mechanism and also limits the search on
views to QWeb views, and adds the check across other HTML fields.
The fields inside ir.action models and mail messages are explicitly
blacklisted, similarly to what exists in 15.0.
This commit also detects non-image attachments which are obtained
through the /web/content/... route.
It also adapts the link within the warning so that the website page of
the blocking object is reached if there is one, otherwise it falls back
onto the backend page of the object.

In 15.0, the list of fields should be obtained from website's
_get_html_fields instead of duplicating its mechanism.

In 16.0, apply the same principle to the menu dependencies which were
restored by 3 and 4.

Steps to reproduce:

• Drop a Text - Image block on a website page/a product page.
• Upload an attachment in that block.
• Save.
• Edit another page.
• In the media dialog, delete the uploaded attachment.

=> Attachment is deleted and page contains a missing image/document.

task-3223788

[robodoo]

Pull request status dashboard.

[xmo-odoo]

So every time an an attachment gets removed we're going to trawl through every HTML field of the database, with a search which would have a hard time being less efficient?

For reference res.partner has an HTML field and we have so many partners the pager has been disabled.

And I don't know how many are non-empty, but I'm not sure postgres implements Boyer-Moore, let alone Commentz-Walter, so my baseline assumption is that it performs a naive full scan of every field for every LIKE (since they're all virtually guaranteed not to match anything ever), of which there are 6. And since those are HTML fields, they would be rather large.

[odony]

To put it differently: it seems we're trying to fix a corner-case problem with an invasive, hackish and badly performing solution. This is a bad trade-off and we generally stay away for that, to keep things simple.

@bso-odoo Either we consider this an important problem and we design a clean solution (which will require some model/architecture changes, not this kind of hack), or we choose to not address it at all.

[odony]

@seb-odoo sorry for the noise, the mention was meant for @bso-odoo or @rdeodoo ;-)

[rdeodoo]

it seems we're trying to fix a corner-case problem with an invasive, hackish and badly performing solution

This use case doesn't seem like a corner-case to me IMHO, far from that.
While clearly the performance should be under control for big databases like Odoo, we shouldn't design our website builder for such databases but for small ones, mainly.

I agree that most people are probably not cleaning/removing their unused image, but for those who do, it's a real use case to not let them break their pages without knowing it.
One could have added an image somewhere, then that image is used later somewhere else (and/or even by someone else). Then you remove it from one page and think it can now be removed from the media gallery, it's important that Odoo does not let you break your other pages without you having a clue about it.

So every time an an attachment gets removed

Note that this is not about any removal of any attachment but only when a user purposely remove an image he uploaded previously from the media dialog: he is clicking on a red trash "delete" icon.

Since we have no way to figure where this image might have been used, we have to check every places which are exposed in the frontend: basically all HTML fields except a few ones we already excluded like mail.message etc.

---

Clearly, while this behavior is mandatory and important (IMHO?), big databases should not be impacted performance wise.
We just discussed it with @qsm-odoo and the easiest would simply to do a search_count before the real search, and if there is more than X records, simply warn that "The image might be used somewhere, be careful" instead of the current behavior where we ensure to the user that it can be safely deleted.

Note that this "badly designed" util method is the one we are using pretty much everywhere:

• Migration flows (obviously to migrate everything correctly as anything from the builder could have been dropped in any html field basically)
• Outdated assets CRON (to check if an outdated cron is still used and disable the asset if not, to lower the visitor assets size and speed up the website serve)
• Page dependencies URL check (when changing an URL of a page, to warn user that some links still points to it and where, with clickable link to go and modify it)

Performance is quite concerning, that's why we came up with 9816b2b (was badly designed in the first place 😬 ) etc.

What do you have in mind when you are saying some model/architecture changes, not this kind of hack?
I think having the behavior change when there is too many records would be ok, especially in stable, no?

[rdeodoo]

My point (see tl;dr below) was exposed in my previous message, as we didn't get any response, we will proceed in that direction. Feel free to hop in should you disagree with it, we are open to discussion.

tl;dr: feature should ideally remain as is for small and medium companies (search everywhere, there won't be many records, exclude tables when needed like mail.message, ensure we provide a reliable warning) but should be tweaked for big ones to not impact perfs (do search_count first, if more than X records -> Don't do the search, just warn that we couldn't provide complete search due to heavy amount of records and link might be used somewhere -> but still do the search for pages/views/menus as it's the core feature).

[bso-odoo]

do search_count first, if more than X records -> Don't do the search, just warn that we couldn't provide complete search due to heavy amount of records and link might be used somewhere

But then, this will be an "a posteriori" warning: "Attachment was removed but it might still have been used in a model that has many records: Product, Job Definition" ?
Or do we want to create some interaction, with a "Delete anyway" button ?

[rdeodoo]

I'd not prevent anything, just add the warning as a hint, as we do for redirects. So a "proceed anyway" I guess.
And should probably be a confirmation modal when you click on delete, it would allow to not delete it right away on top of having a better UX that the current implementation with the message inside the media dialog 👍

[rdeodoo]

As discussed, summary:

• Missing sudo
• Missing active_test=False
• In 15.0 forward port, adapt your TODO with our utils # TODO In 15.0, obtain additional fields from website._get_html_fields.
• In 16.1 forward port, adapt https://github.com/odoo/odoo/commit/15c350ae47332d28f64f6a552b5c5d6930054e7d which will share the same code + fix the missing sudo+with_context=False
• Find a way to detect links to non accessible records and replace the display_name by id to not leak info so everyone is happy. You will probably need to go with exists() on the recordset un-sudo or something.

I made a summary on the 16.1 fix attempt which was done at #123014 (comment) but which will be handled in this PR forward port instead

[bso-odoo]

@rdeodoo It seems I forgot to ping you when it was done. The changes are in a second commit to be fixup.
Now there are four outcomes of the deletion:

• no reference found
• references found and displayed to the user
• too many records in model => display general message about the model
• insufficient rights (but reference found with sudo) => display general message about presence in inaccessible model records

[rdeodoo]

Let's try to merge it tomorrow, since this will need a few more hours to later handle the forward port.
Note that you don't especially need to apply this mini review before we discuss it.

@qsm-odoo Your input would be more than welcome tomorrow, let's try to discuss this PR and all the related context about it.

[bso-odoo]

@rdeodoo Pushed a new version with:

• combined results: specific records, warnings about occurrences with missing access or too big tables (did not remove it yet)
• find_in_html_field is now a tools in web_editor
• it relies on _get_html_fields which is defined by web_editor on the base model
• _get_html_fields's override by website collects all HTML fields from the model definition table
• a new route is used for the removal of attachments, the old one is deprecated

Still to be discussed:

• in stable: maybe we do not want the "big table" thing, but instead explicitly define a few fields to examine
• in master: maybe we want a dedicated model that keeps track of references updated using the website builder - and limit the search to such a table

[rdeodoo]

Until now, the changes were done in Odoo 14 as:

• we had no better technical solution for now
• the modified code is (almost) exactly the same in all supported versions
• the diff is retro-compatible so we might as well do it in 14 fixing some different flows from 14 to master at the same time

Since @xmo-odoo and @odony were not convinced at all by the idea of auto discovering html field (because of obvious perf issues) but I was convinced we needed to have an accurate check of those "is the ressource used somewhere?", I actually thought about it again and tried to figure a possible improvement in master that would not impact perfs while still being as precise as possible.

The idea is to have a new table which would keep track of every field modified through the website builder.
Technically, there are only a very few endpoints when modifying a record, those endpoints would simply populate the table whenever a record is modified.
We would then simply need to query this very short (in comparison with every single html field of every single record of the database (except some exception like mail.message)) table to figure the records to check in order to know if the ressource is used somewhere.
There is some limitation to this idea, like compute store field which copy value of another HTML field (like sale.order.line.website_description), but this seems very acceptable compare to other solutions.

The idea has been challenged and seems like a good compromise fitting those needs.

Since it will require a deeper refactoring, it will only be done in master, making it ok to not consider the stable versions.

---

As a reminder, the flow where we need to know where/if a ressource is used in some HTML field are:

• Snippets version check to know if we can disable the outdated snippet assets
  This one is critical, we need to have an almost perfect check to not disable an asset that would actually be used.
• Page dependencies to know if it's safe to change a page url
  This one is important, we don't want to let the user modification lead to 404 links without him knowing, this is bad for the visitor UX.
• Image deletion to know if it's safe to delete an image
  This one is a nice to have, it's not ideal to have a 404 image instead of a real image, especially if the image is really important (part of an explanation or really impactful design wise) but it's more acceptable than a dead page.
• Property field deletion to know when you delete a property field that it was used in a website form
  This one is unsure and still related to a not yet merged IMP in master.
• Migration: Did not yet consider this one, just thought about it, but we have many calls to get_html_fields in the upgrade repo where we go through all HTML fields to apply a modification.

cc @pierre-pde @qsm-odoo

[bso-odoo]

@rdeodoo FYI Master PR with the new table is #128153. I adapted Snippets version check and Page dependencies so that they rely on it.