search

agritheory / inventory_tools

A collection of features to streamline and enhance inventory management and manufacturing workflows in ERPNext.
https://agritheory.com/documentation/inventory_tools
Other
24 stars 13 forks source link

ci: track overrides for purchase cycle (PO/PI/PR) #89

Closed HKuz closed 6 months ago

HKuz commented 6 months ago

Addresses #86

I think I grabbed the right commit for each method/file (I checked the blame timeframes for the method in ERPNext's version-14 branch to make sure nothing major changed recently, and used the most recent commit for the given file).

One discussion point - I did NOT include the tracking docstring in methods for validate/on_cancel/on_submit for these files because we weren't changing the ERPNext code in them. Our pattern adds other methods to call, then calls super. We should decide to:

a) keep code as-is, don't include tracking for that scenario (i.e. it how the current PR does it) b) keep code as-is but include the tracking docstring to those methods c) refactor the current code to call those methods via hooks' doc_events instead of overriding/adding our method calls/calling super, then we wouldn't need the tracking docstring

agritheory commented 6 months ago

@fproldan This looks right to me, can you give it a quick check?

fproldan commented 6 months ago

@HKuz

a) This is ok, as long as we are not modifying Frappe/ERPNext code I don't see the need to track it. b) It doesn't hurt to add the docstrings but surely we will get most false alarms in these cases. c) It's a good option but I think that hooks methods execute after the original code and we might need to execute our before in some cases. (I'm not sure tho)

I would go for option "a"

agritheory commented 6 months ago

@HKuz Can you raise a version-15 version of this also?