Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPN uniqueness evaluation including revision #4618

Closed
1 of 2 tasks
gunstr opened this issue Apr 17, 2023 · 30 comments
Closed
1 of 2 tasks

IPN uniqueness evaluation including revision #4618

gunstr opened this issue Apr 17, 2023 · 30 comments
Labels
part Related to Part models plugin Plugin ecosystem question This is a question

Comments

@gunstr
Copy link

gunstr commented Apr 17, 2023

Please verify that this feature request has NOT been suggested before.

  • I checked and didn't find a similar feature request

Problem statement

Would it be possible to include the Revision in the IPN uniqueness evaluation in the same way as for part description?

In all tables I then see revision duplicated in both the IPN and Revision column, If I understand correctly the "last used serial number" would then also be evaluated for the base number and not restart for each revision.

Sorry it there is already a setting controlling this that I have not found.

Suggested solution

Include the Revision in the IPN uniqueness evaluation

Describe alternatives you've considered

Currently I need to include the revision in the IPN also.

Examples of other systems

No response

Do you want to develop this?

  • I want to develop this.
@gunstr gunstr added enhancement This is an suggested enhancement or new feature triage:not-checked Item was not checked by the core team labels Apr 17, 2023
@matmair matmair removed the triage:not-checked Item was not checked by the core team label Apr 17, 2023
@matmair
Copy link
Member

matmair commented Apr 17, 2023

Hi there @gunstr
It would probably be possible with some small adaptations in core code, but not with plugins currently.
You are correct that it would probably need an adaption for the serial counter. I think that could be a challenge as we allow any chars in the revision field and we would probably need a migration for that.
Thoughts @SchrodingersGat

@matmair matmair added the part Related to Part models label Apr 17, 2023
@SchrodingersGat
Copy link
Member

@gunstr I'm not fully sure that I'm following what you are asking for here

Currently the "PART_ALLOW_DUPLICATE_IPN" setting just allows duplicate IPNs across part, the "description" of the part is not checked as part of this?

@gunstr
Copy link
Author

gunstr commented Apr 20, 2023

@SchrodingersGat Sorry it was a typo from my side that made this confusing. It should be "name" - not "description"... Let me try to explain:

For each new part the name has to be unique. If you add a revision multiple parts with the same name can be created. Correct?
Also (and here I'm not sure I have the complete picture) those revisions will be treated as completely separate parts with their own BoM, pricing etc. Only one exception I have found is that the serial counter will use the same number pool for all revisions - perfect!

I use IPN's and as i need them to be unique I set PART_ALLOW_DUPLICATE_IPN to false which is fine for most parts.

Some parts also have different revisions and thus I currenty have to assign a new IPN for each revision.

So my question is if it would be possible to allow one IPN for multiple revisions also when PART_ALLOW_DUPLICATE_IPN is set to false in the same way as one name can have multiple revisions?

More clear now?

@gunstr
Copy link
Author

gunstr commented Apr 24, 2023

@matmair I found the section in the code that validates the IPN and it confirms that with PART_ALLOW_DUPLICATE_IPN set to false the same IPN cannot be used for different revisions.

I was thinking a bit more on this but I have to admit I'm a bit on deep water here. If I set PART_ALLOW_DUPLICATE_IPN to true instead can I then create a plug-in that validates that the IPN+Revision is unique? You mentioned it's probably not possible. Is that because the validation mixin cannot be used to validate the uniqueness vs other parts so that would be a dead end? I looked at the validation plugin samples, and it seems to only act on the actual part and cannot reference other parts in the database.

@matmair
Copy link
Member

matmair commented Apr 24, 2023

@gunstr we do not expose the revision of the current part to the validation function so you can not validate it.

@gunstr
Copy link
Author

gunstr commented Apr 24, 2023

@matmair Thanks, then I understand why a plugin is not an option.

@SchrodingersGat
Copy link
Member

@gunstr @matmair I would disagree here, I think that you certainly can have a custom validation routine for the IPN which operates exactly as you require.

From the code (linked above):

result = plugin.validate_part_ipn(self.IPN, self)

And from the plugin code:

def validate_part_ipn(self, ipn: str, part: part.models.Part):

The "part" instance is passed to this function, so you have access to any attribute of the part. Writing a custom plugin here would be the correct approach IMO

@SchrodingersGat SchrodingersGat added question This is a question plugin Plugin ecosystem and removed enhancement This is an suggested enhancement or new feature labels Apr 26, 2023
@gunstr
Copy link
Author

gunstr commented Apr 26, 2023

Thanks a lot @SchrodingersGat. I will give it a try to write a plugin in the coming days and let you know.

@gunstr
Copy link
Author

gunstr commented Apr 29, 2023

@SchrodingersGat I have created a basic implementation for an IPN validation plugin and I can check various aspects on the IPN itself and also other attributes on the new part like that there is a revision assigned by accessing part.revision.

But I cant figure out what mixin I need to add to the plugin to be able to query all existing parts in the database to validate that the IPN+revision on unique. I believe a test similar to what you do for the name in the core code should be done, but then only for IPN+revision

# Ensure unique across (Name, revision, IPN) (as specified)
if Part.objects.exclude(pk=self.pk).filter(name=self.name, revision=self.revision, IPN=self.IPN).exists():
raise ValidationError(_("Part with this Name, IPN and Revision already exists."))

Appreciate if you can guide me in the right directions.

@SchrodingersGat
Copy link
Member

But I cant figure out what mixin I need to add to the plugin to be able to query all existing parts in the database to validate that

I don't think you need to add any other mixin? You should have direct access to make the same database calls as you provided in your example above?

@gunstr
Copy link
Author

gunstr commented Apr 29, 2023

Oh, that I did not understand.

I would assume I then at need some more import(s) on top of what you have in the IPN validation sample the be able to do a similar call as you do in the part/models code? I tried and it seems I cannot use the Part class right now, so I have obviously missed something...

from datetime import datetime
from django.core.exceptions import ValidationError
from plugin import InvenTreePlugin
from plugin.mixins import SettingsMixin, ValidationMixin

@gunstr
Copy link
Author

gunstr commented Apr 29, 2023

I added
from part.models import Part
to the plugin.

With the quick testing I have done it seems to work like a charm. I can now set PART_ALLOW_DUPLICATE_IPN to true and the plugin will raise an error if the combination of IPN+Revision is already existing!

The validation method in all simplicity is only:

def validate_part_ipn(self, ipn: str, part):  
    if Part.objects.exclude(pk=part.pk).filter(revision=part.revision, IPN=ipn).exists():   
        raise ValidationError("Part with this IPN and Revision already exists.") 

@SchrodingersGat
Copy link
Member

Glad you got it sorted with something simple :) if the plugin is public would you be willing for us to add it to the plugin samples page?

@gunstr
Copy link
Author

gunstr commented Apr 30, 2023

Sure, I'm happy to share. I have the plugin on my local installation right now, will fix a git repo and let you know when it's done.

@walkermc20
Copy link

Were you able to get this uploaded? We're in the same boat and would love to implement something that's already complete. 💯

@wolflu05 wolflu05 assigned wolflu05 and unassigned wolflu05 Jun 2, 2023
@gunstr
Copy link
Author

gunstr commented Jun 3, 2023

It's a bad conscience of mine but give me a couple of days to fix it. I believe I had a bit too high ambition to clean it up and document but If you are OK with it I will just upload the code in all it's simplicity as a start. I have used it for a month now and it works perfectly fine :-)

@SchrodingersGat
Copy link
Member

@gunstr looking forward to seeing what you've come up with :)

@gunstr
Copy link
Author

gunstr commented Jun 4, 2023

Now the plugin is uploaded, you find it here. I have only installed the plugin locally as I have never done any PIP packaging. This could be a good case for me to learn but I thought it's better not to wait until I have done that.

@walkermc20 appreciate if you can test and and let me know if it works for you also.

@SchrodingersGat However, I tried to move it out from the src/InvenTree/plugins directory in my installation by pointing the plugins_plugin_dir variable in the config file to a custom directory where I put the plugin. But when I do so it will not be found by the system. I can't really figure out what I'm doing wrong - are you aware of any common mistakes that might cause this? It's not really a problem for me to keep it inside the src tree but as you do not recommend to do so in the documentation I thought it would be better to get it working...

As a little side note I found that the documentation refers to plugins_plugin_dir as the variable name while in the config_template.yaml is plugin_dir.

@matmair
Copy link
Member

matmair commented Jun 4, 2023

@gunstr cool plugin!
If you want a hint about Python packaging you can take a look at this repo or the meta-package I use for packaging and creating new plugins.

@ghost
Copy link

ghost commented Jun 5, 2023

I am working on installing this plugin with @walkermc20 . I have placed the validate_ipn_unique directory in my opt/inventree/Inventree/plugins and have adusted my config.yaml file so that plugins_enabled: True.

Yet I am still getting an External plugins are not enabled for this InvenTree installation error on my settings/plugins page. Any advice?

@SchrodingersGat
Copy link
Member

@andrewwetzel23 could be a few things:

  • Have you restarted the server?
  • Anything set in your environment variables will override the values in config.yaml

@ghost
Copy link

ghost commented Jun 5, 2023

Yes, I have restarted the server using: docker stop inventree-server and then docker start inventree-server.

Unless there is something out of the box that would overwrite those values it shouldn't be. I haven't changed to much from the boiler plate docker install.

Is there anything I need to change in the setup.py script? I saw there are function calls there that look like they may effect plugins.

@gunstr
Copy link
Author

gunstr commented Jun 6, 2023

My installation is Bare Metal (on a VM), unfortunately I can't be of so much help on Docker. The only issue I had was that I could not move the plugin outsisde the InvenTree source code as I mentioned above.

@ghost
Copy link

ghost commented Jun 6, 2023

Understood. I think my issue is probably more of a config problem than one with your plugin. Going to take a deeper look at it today. Probably some setting I missed somewhere.

image

@SchrodingersGat
Copy link
Member

@ghost
Copy link

ghost commented Jun 7, 2023

Thanks, I now have plugins enabled, but after a restart and with the validate_ipn_unique directory placed in the /opt/inventree/InvenTree/plugins directory, it does not seem to be found. This is my first experience with plugins in InvenTree. I should just show up as another plugin underneath the Plugins page in the Settings sections correct?

EDIT:
I found the correct folder to put it in which was in the mounted docker storage folder as the documentation showed.
My current issue is that I have been unable to list the plugin as active. I have attempted to activate it through the admin panel and it shows that it is active in the admin but when I go out to settings it still shows as inactive.

EDIT2:
I had to set it as active on the admin page and then restart the server. Perhaps the settings page could have an "activate" button for easier access.

@matmair
Copy link
Member

matmair commented Jun 7, 2023

@andrewwetzel23 that was actually already developed but is not released at the moment.

@wolflu05
Copy link
Contributor

wolflu05 commented Jun 7, 2023

Ref: #4964

@gunstr
Copy link
Author

gunstr commented Jun 15, 2023

@SchrodingersGat I guess this issue can be considered as resolved now and can be closed? If you want to add a link to the plugin to the samples page please feel free to do so.

@matmair
Copy link
Member

matmair commented Jun 15, 2023

@gunstr we can add it to the plugin repo if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part Related to Part models plugin Plugin ecosystem question This is a question
Projects
None yet
Development

No branches or pull requests

5 participants