link titles with zimfarm recipe#343
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 81.92% 82.58% +0.66%
==========================================
Files 60 63 +3
Lines 3203 3445 +242
Branches 333 356 +23
==========================================
+ Hits 2624 2845 +221
- Misses 484 499 +15
- Partials 95 101 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@benoit74 , i'm guessing we need a maint-script to populate the db with |
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Yes, we need a maintenance script to populate the DB. This is probably not an easy one to build since it needs to source its data from Zimfarm. Let's try to build something and we will iterate.
| ForeignKey("title.id", ondelete="CASCADE"), init=False | ||
| ) | ||
| flavour: Mapped[str] | ||
| recipe_id: Mapped[UUID | None] = mapped_column( |
There was a problem hiding this comment.
Let make this not nullable for now. Recent (last friday) discussions with @kelson42 led me to consider we might always have books coming from Zimfarm in the end. And at least this is what we have for now.
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Before reviewing the full PR, I would like to also be able to test it locally.
Did you tried to run the maintenance script on a recent copy of both CMS and Zimfarm? Did it produced meaningful warnings or just too many? We should have only few, or it probably means we have a broader issue.
And how long did it take, is it "reasonable"?
I also feel like the script should take into account known bad flavours like _maxi which should be transformed automatically to maxi (you can code something like _<anything> becomes <anything>
I did few tests locally and I feel like "fix recipe" UI should be greatly simplified, see inline comments.
Besides this PR, we will need more screen to manage existing recipes (outside of any book issue).
| ) | ||
|
|
||
|
|
||
| def test_update_zimfarm_recipe_dissociate_recipes_from_flavours( |
There was a problem hiding this comment.
We should also have following test pass:
def test_update_zimfarm_recipe_dissociate_recipes_from_flavours(
dbsession: OrmSession,
create_zimfarm_recipe: Callable[..., ZimfarmRecipe],
create_title: Callable[..., Title],
):
recipe1 = create_zimfarm_recipe()
title = create_title(zimfarm_recipe=recipe1)
# "mini", "maxi" and "nopic" flavours belong to recipe1
mini = TitleFlavour(flavour="mini")
mini.title = title
mini.recipe = recipe1
maxi = TitleFlavour(flavour="maxi")
maxi.title = title
maxi.recipe = recipe1
nopic = TitleFlavour(flavour="nopic")
nopic.title = title
nopic.recipe = recipe1
dbsession.add_all([maxi, mini, nopic])
# recipe2 just arrives (with mini now produced by it)
recipe2 = create_zimfarm_recipe()
dbsession.flush()
update_zimfarm_recipe(
dbsession,
recipe=recipe2,
flavours=["mini"],
title=title,
current_recipes={recipe1.id, recipe2.id},
)
# recipe1 still has maxi and nopic
assert (
count_from_stmt(
dbsession,
select(TitleFlavour).where(TitleFlavour.recipe_id == recipe1.id),
)
== 2
)
# recipe 2 has mini
assert (
count_from_stmt(
dbsession,
select(TitleFlavour).where(TitleFlavour.recipe_id == recipe2.id),
)
== 1
)
dbsession.refresh(recipe1)
assert recipe1.title_id is title.id
dbsession.refresh(recipe2)
assert recipe2.title_id == title.id
I'm not sure how (see other comment on backend implementation)
There was a problem hiding this comment.
Thank you very much for the test. It expose a flaw in my db model and the way I link title with recipe.
| current_recipes: set[UUID], | ||
| create_event: bool = True, | ||
| ): | ||
| """Update a recipe to be associated with the title and flavours. |
There was a problem hiding this comment.
This algorithm is not totally accurate and can probably be simplified, see other comment on test case.
Simplification could be to simply iterate over all flavours passed, create or fix corresponding title flavours if needed (recipe is different) and also update recipes (remove title_id from old recipe, add title_id to current recipe) in the same go. And finally, check that all old recipes are not in the list of current_recipes.
Btw, maybe it would be simpler and more intelligible to simply pass old_recipes in the API call, and checking that the old recipes we've collecting during flavours processing do match the passed list.
We do not need to (and should not) touch other flavours not passed in this API call because produced by other recipes, they are "probably" just fine (or another API call will update them)
| <v-card variant="elevated" class="mb-4"> | ||
| <v-card-text> | ||
| <p class="text-body-2 mb-4"> | ||
| Browse and select the Zimfarm recipe that you want to use for this book's |
There was a problem hiding this comment.
Why letting user select a recipe? We know this book has been produced by a given recipe, user should not be able to choose a recipe, only fix this book recipe configuration.
Btw, I realize we do not store this information in the book. We probably need to (and this is the reason you had to ask user again for this information).
| </div> | ||
|
|
||
| <div class="mb-4"> | ||
| <span class="text-subtitle-1 font-weight-bold">Expected Flavours</span> |
There was a problem hiding this comment.
This is really too complex IMHO. And it does not allows to set a blank flavour (book without flavour).
Once title is selected, why not simply:
- listing all current title flavours
- if book flavour is already in title but with another recipe, propose to update title flavour
- and if book flavour is not "known" in current title, propose to add it to the title
And below, that, display consequences on recipes (basically the content of the API request):
- which title the current recipe is associated with
- which flavours the recipe is hence expected to produce (prior ones + current one)
- which recipe will not have anymore the corresponding flavour / title associated
I ran the script on prod zimfarm but i didn't let it complete because there were too many. If I ran it locally, i would have had more than 10k tasks across 2k recipes. Most of it are just warnings because title flavour already exist. I couldn't run locally because for some reason, i can't have two database containers in two different folders running . Don't know if it has to do with the different postgres versions of CMS and Zimfarm |
These are not warnings but debug logs. I was specifically referring to warnings like abnormal unexpected things.
I can run both fine on my machine, side-by-side. We need to really find a solution to run this migration (and any migration in fact) before releasing to production. We can't just hope the migration will have the intended effect, especially on very large migrations like this one which could have multiple bugs linked to forgotten conditions between Zimfarm and CMS status. |
Finally fixed it with the |
Ran the script and so far, the only warnings I have been getting on my copy of zimfarm DB is: [2026-06-19 00:04:41,589: WARNING] Task a60ba59e-8f17-47ed-8899-2064e1f4a8c0 metadata is missing keys: Name
[2026-06-19 00:04:41,606: WARNING] Task 2ff2e7e8-204e-4b61-8f5c-6e16da10e0e7 metadata is missing keys: Name
[2026-06-19 00:04:41,623: WARNING] Task 667ba7ab-98d9-4a89-88f0-7a6b0052f4f0 metadata is missing keys: Name
[2026-06-19 00:04:41,640: WARNING] Task 75eb9de0-41c9-462b-826a-6ac60202eb3f metadata is missing keys: Name |
Ran the script and so far, the only warnings I have been getting on my copy of zimfarm DB is: [2026-06-19 00:04:41,589: WARNING] Task a60ba59e-8f17-47ed-8899-2064e1f4a8c0 metadata is missing keys: Name
[2026-06-19 00:04:41,606: WARNING] Task 2ff2e7e8-204e-4b61-8f5c-6e16da10e0e7 metadata is missing keys: Name
[2026-06-19 00:04:41,623: WARNING] Task 667ba7ab-98d9-4a89-88f0-7a6b0052f4f0 metadata is missing keys: Name
[2026-06-19 00:04:41,640: WARNING] Task 75eb9de0-41c9-462b-826a-6ac60202eb3f metadata is missing keys: NameTook 3hrs in total |
|
Thank you, great news for the migration script! Regarding API / UI, I realize again this is just really complex. I'm sorry but I will need some time to think twice again about this change. |
updated maint-script and api/ui to view recipe history |
|
We've discussed the matter with @kelson42 last Friday and the output is that he considers our approach is significantly wrong. His main point was that we should not ask users to re-enter things which can be assumed from book content ; we should just move the book to quarantine or staging, and have buttons to automatically do what is needed. For instance it means that for association of book with title, we should have a "set title" button on the book which proposes to either choose an existing title or create a new one, and automatically assume everything coming from the book is correct once user has chosen the correct title. I expect all this should be done in the backend, preferably in a single API call for consistency, but then I'm not sure what to show on the UI so that user still knows what he asks the backend to do. A second point what that we should not show / speak about Zimfarm recipe in the CMS since it is a Zimfarm attribute. I absolutely don't know what to do from this information. A third point was this is plain simple to him and he does not understand why we are speaking about the matter. I don't know what to do of this statement. @elfkuzco do you know how to move forward from these statements or do we need to arrange yet another discussion with @kelson42 |
I believe we already have a button to add book to title and buttons to create title. Just not shown in the issue tab.
Given the issue was to link zimfarm recipe and cms title, if we don't display the information in the API/UI, i'm not sure how issues would be fixed. For example, if this information is completely hidden and we create recipes as they arrive, we can have a conflict where a book arrives and we don't create a recipe because the recipe already exists. But the recipe points to a different title from the one than the matching title. Such books would just remain with |
2 participants
Rationale
This PR redefines the way books are attached to titles. When books arrive, their recipe details are retrieved from the zimfarm notification and are only linked to title when the recipe exists in the database and the title (or it's flavoours) to be associated also points to the same same recipe. All other scenarios lead to the books not being associated with a title and an issue
recipe issueadded to it and they are forced to staging. Accordingly, a UI is added to resolve book issuesChanges
recipe issuesThis closes #344
This closes #339
This closes #255