search

mpkasp / django-bom

A simple bill of materials app built using the django web framework.
GNU General Public License v3.0
105 stars 40 forks source link

Sellers informations should be revision dependent #60

Open ks156 opened 3 years ago

ks156 commented 3 years ago

I'm not sure I'm using django-bom the way it has been imagined, so maybe this is not an issue, but more a feature request.

When I create a BOM, I want to add sub-parts with price. This allows to have a good view of my product costs. When I send the device into production, I want to release the revision (say revision 1), and create a new one (revision 2) to prepare the next production.

Even if there's no changes on the BOM regarding the part list, the parts prices will likely changes. If I adjust the parts prices in this new revision, the overall price as well as each part of the revision 1 will be impacted by the new prices.

To summarize the problem, the manufacturer parts and the sellers parts should be dependent of the part revision, and not the part (on a database point of view). This will allows to get statistics of price evolution from one revision (in my case, revision = production run).

Once a part is released, all its attributes (including the sellers data) should be freezed I think. This is currently not the case.

Before working on these changes, I think a discussion about this feature and its impacts is needed. Also, this is maybe not in the implementation philosophy and thus this feature should be rejected.

ks156 commented 3 years ago

By the way, just saw now issue #12, which is probably related to this one

mpkasp commented 3 years ago

Agreed on this one and I'm open to the change. SellerParts being tied to Parts instead of PartRevisions is kind of tech debt that as the tool grew, some places got left behind. #12 is related but more specific to tracking inventory, not so much sourcing information so I'm ok leaving this one open. If you want to take a stab at this that would be great!

ks156 commented 3 years ago

Ok, I can implement these changes. I'm a bit worried about the database migration, especially on existing databases. I'll check that during the implementation

mpkasp commented 3 years ago

Sounds good and no worries. Happy to check it out on my end too when you're close. Could be helpful on your end to use some of the test helpers.py to generate some fake data in a local database.

ks156 commented 2 years ago

Hi @mpkasp. As you maybe have noticed, I didn't have time so far to implement this. I'll maybe have a little more time in the next few days. As I don't have a deep knowledge of django, and even less about django-bom, do you have some advice that could help me with this?

mpkasp commented 2 years ago

Hi @ks156 Sure, you're going to want to start in models.py and looking at the relationship of SellerPart. Right now SellerPart maps to ManufacturerPart which then maps to a Part.

The thing we'll likely need to change is that a ManufacturerPart needs to map to a PartRevision instead of a Part.

This will probably have UX implications. Any time we bump a revision, all seller info and manufacturer part info will be lost unless we update the UX appropriately. I imagine something like a question when bumping the revision along the lines of "do you want to keep the same Manufacturer and Seller information?"

Hopefully this helps as a starting point.

ks156 commented 2 years ago

Hi @mpkasp. As you may see, I'm still very busy and time is still hard to find to implement changes on django-bom.

Anyway, after having used it a lot over the last weeks, I figured out that linking seller_parts to part_revisions is maybe not the best solution. This change would require a lot of work (UX implications are big, and I have some ergonomic points that I don't really how to solve).

Initially, I thought that revision could be used for different production batches, but it also appears to be a bad way.

I'll propose a merge request with the following change :

If no sellerparts are set as preferred, the old method to determine the best part is used (based on price and quantity). If more than one sellerpart is set as preferred, the first found is used. This is not perfect, but this is a simple change that helps me a lot.

mpkasp commented 2 years ago

Makes sense, PR looks generally good, just a few questions I left on it.