I’m following up on this topic — I’ve finished the verification for the PR that improves the energy tracking recalculation. I think you can review:
This will be followed by the next PR which adds clearer logging for the user, notably to see where the last launched tasks were at if a Gladys restart or an issue occurred during a recalculation (avoiding having to redo the entire recalculation …):
Could you describe in more detail what this PR does?
Since this touches the core, these are fairly sensitive changes.
I’d like to understand precisely the scope to be able to properly assess the impact.
During development I spent several months improving the reliability of the calculations, and it’s vital to me that the calculation isn’t affected 
You’re right to ask for the details, I completely understand. And if possible, I’d prefer that you also test it by running it alongside your production environment to properly verify. That’s what I did to properly assess correct operation. I didn’t find any scenarios where it crashes. But for example, I don’t have any Zigbee energy. Even though there’s no reason it should be different, that would cover everything.
The overall diff is large (+3203 / -428), but the functional scope I’m targeting in this PR is mainly the following:
calculate-cost-range)calculate-consumption-from-index-range)feature_selectors).YYYY-MM-DD dates on the controller side (with BadParameters if the format is invalid).destroyStatesBetween) before recomputation.destroyStatesFrom).ENERGY_INDEX_LAST_PROCESSED at the end of processing on partial/selected recalculations to avoid damaging the global cursor.wrapperDetached for long recalculations (from beginning / range), to immediately return a job_id.job_id is returned.range endpoints if a date is provided, otherwise the from-beginning endpoints.contracts.calculateCost remains the same).I have:
I have « only » modified the input scope (selected features, date range) and the technical framing of the recalculations (targeted purge, jobs, API/front).
And to tell you the truth, at the start of the week I almost closed the PRs for this topic and wrote to tell you that I was leaving it in your hands.
I was afraid it would take you longer to review it than to do it yourself. That on top of that visually it might not suit you. And that as a result you’d probably be better off doing it the way you think.
But I’m torn between that and the fact that there are still a few of us waiting. And the fact that I can see you’re busy with many other things.
So I decided to take the approach of proposing it and leaving it up to you. And you’ll give me your feedback / impressions. 
Thanks for the details, I’ll let you know when I’ve had a chance to take a look
I had a quick look at the PR and it seems to touch more things than just the date re-calculation.
For example, the first four files appear to be leftover from another PR:
For the business code, the PR is quite large and touches so many elements that it’s hard to be sure it doesn’t change the calculation algorithm.
There’s even a new logic introduced with new concepts (like shouldRestoreLastProcessed), so it’s clearly not a trivial PR.
Yet intuitively, if I think about the idea for two minutes, I struggle to understand why it’s so complex.
I wonder, unfortunately, if your intuition is right:
It’s really a shame, because I actually agree with the result and the UX seems good to me. The problem is that the code is huge: I can’t merge without reading it entirely, understanding the new concepts, and running all the necessary tests… 
What do you think?
Hello @pierre-gilles
Thanks for starting to look.
The PR branch started from the Tasmota branch, which has had quite a few changes following our discussions.
After updating the PR once Tasmota was merged, I went over it again, but I missed the file ‹ front/src/components/device/index.js › which still includes the creation of device conso/cost => I don’t understand how I missed it; it’s the first file and it’s a big one. It’s removed
front/src/components/device/UpdateDevice.jsx and front/src/components/device/UpdateDeviceFeature.jsx were adding a shortcut from features that had associated consos/cost to go directly to the energy tracking page. That can be proposed later; I removed it.
front/src/components/drag-and-drop/DeviceListWithDragAndDrop.jsx: the component is used in the selection of devices to be recalculated. There was a need to add an inability to change the name for this use.
I agree, this is not a trivial PR on that point.
Originally, I considered duplicating the logic into a separate recalculation engine to avoid depending on ENERGY_INDEX_LAST_PROCESSED.
But I deliberately avoided that: we’d have had two parallel implementations (incremental vs recalculation), so more complexity with a risk of divergence over time.
Today I added calculateConsumptionFromIndexRange as an orchestrator (date bounds, cleanup, window loop).
I wanted to keep the actual calculation of each window by always going through calculateConsumptionFromIndex, which itself uses ENERGY_INDEX_LAST_PROCESSED.
So, shouldRestoreLastProcessed is used to protect the live incremental cursor during partial/retrospective recalculations. Because before, the calculation always went « to the end ». But here the goal is to be able to add an old contract price and launch the recalculation on that range only. If that range was a year ago, without this the next 30-minute calculation would have resumed over a complete year instead of restarting at the last real tracking run.
Without restoration:
With restoration:
Because this cursor is used by the index consumption calculation.
In short, it’s a safeguard to keep a single maintainable engine, without side effects on subsequent incremental runs.
Of course I may be wrong about the logic, but in particular on this point I worked with the AI and tried to turn it every which way to be sure it’s a good way to do it (not necessarily the best; that’s what the review is for!)
I’m well aware of that, and I told you from the start of this PR. It touches a sensitive element, and I was hoping that you could run it on an instance next to your prod to be sure that the calculations (which have not been changed) are correct even after range recalculations, etc.
For the code, for your information these are the largest tests:
Have a good weekend and a good run
Hi @pierre-gilles,
Not knowing if you’re in class or when you’ll get to reviewing this PR, I thought it would be good to relaunch a full review by @coderabbitai because I notice he hasn’t gone over it again after a few passes.
And given the number of commits made since his first review, I thought it would be good to do a full one on the complete PR: https://github.com/GladysAssistant/Gladys/pull/2413#pullrequestreview-3943430667
I think he raised some good points. If you haven’t started your review, may I push the changes? Otherwise, what do you prefer?
I haven’t looked yet, you can modify the PR
Ok, it’s done.
