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

Extend API for labels and reports to allow creating new items #5982

Merged
merged 4 commits into from
Nov 26, 2023

Conversation

miggland
Copy link
Contributor

Label and Report endpoints don't have the ability to create new items.

This simple change fixes that, but are there other implications? Is there a reason this is not present yet?

PR created to run tests properly, may show issues :)

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit e5e7579
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/656303a10586020008dd9aec

@matmair
Copy link
Contributor

matmair commented Nov 24, 2023

@miggland I don't think I understand how you are trying to achieve your stated goal with this diff.
To create a new label you would need to have the templates for them present on the server, I do not see how that should be done with this change. Can you expand on that?
There should also be integration tests to ensure that uploading and validating the files works properly as there are some file operations involved with that that could break.

@wolflu05
Copy link
Contributor

Is there a reason why this is a file and not stored in the database? In regards to #2777, #2585 storing them in the database would simplify things.

@SchrodingersGat
Copy link
Member

@miggland as @matmair states above this will not actually allow you to create a report or label due to the actual template file being exposed to the API. This is not insurmountable, but I'm not sure if this approach is what we want.

@wolflu05 you are right it would be simpler. The current approach (files) is in line with the general Django file-based templating system, and partially also because that's just what I knew how to do way back when it was first implemented.

I think that the live editor approach is the end goal here. Uploading and re-uploading to make a small change is really annoying, and the whole report templating system would be way more useful if you could see the effects of your changes live. The new platform UI should make this not too hard - we will have to change the API anyway to make this work.

Either we keep the templates stored in files, and expose the files to the API, or we store the template data in a database field.

The issue with a DB field is that we have to specify a maximum field size, and also I'm not sure what implications there may be depending on database encoding, field type, etc. Using files also allows the templates to easily call in data from other templates (e.g. create a base template or snippet which can be reused in other templates).

@miggland
Copy link
Contributor Author

I'm afraid it seems @matmair is assuming he knows how things work without actually trying it out..

Let me show you..

Preparation

Locally, make a new file Newreporttemplate.html, with content:

{% extends "report/inventree_po_report_base.html" %}

{% block header_content %}

    <div class='header-right'>
        <h3>FANCY NEW TEMPLATE</h3>
    </div>

{% endblock header_content %}

Below, I obviously used the correct username and password in the curl calls.

Before this PR

Try to upload the template through the API

$ curl -X POST --basic -u user:pass -F template=@Newreporttemplate.html -F name=FANCY -F description="New fancy template" https://demo.inventree.org/api/report/po/
{"detail":"Method \"POST\" not allowed."}

OK, creating a new item is not allowed.

Let's try changing the template of an existing report. The api-doc shows me there is a PUT endpoint for this:

$ curl -X PUT --basic -u user:pass -F template=@Newreporttemplate.html -F name=FANCY -F description="New fancy template" https://demo.inventree.org/api/report/po/1/
{"pk":1,"name":"FANCY","description":"New fancy template","template":"/media/report/report_template/purchaseorder/Newreporttemplate.html","filters":"","enabled":true}

Ah look, I uploaded a local file from my computer through the API and it was placed on the server in the right place!

Print the report:

curl --basic -u user:pass https://demo.inventree.org/api/report/po/1/print/?enabled=true\&order[]=10 -o Report.pdf

Look what I get:
image

With this PR

$ curl -X POST --basic -u uesr:pass -F template=@Newreporttemplate.html -F name=FANCY -F description="New fancy template" http://DEVSERVER:8000/api/report/po/
{"pk":3,"name":"FANCY","description":"New fancy template","template":"/media/report/report_template/purchaseorder/Newreporttemplate.html","filters":"","enabled":true}

Ah look, creating a new report with my local template works!

Print the report:

curl --basic -u user:pass http://DEVSERVER:8000/api/report/po/3/print/?enabled=true\&order[]=1 -o Report2.pdf

Behold:
image

Summary

It isn't clear why PUT to an existing object is allowed, but POST for a new object currently is not. The rest of the API follows this logic.

There should also be integration tests to ensure that uploading and validating the files works properly as there are some file operations involved with that that could break.

I don't see how this is relevant to this PR. This PR only tries to make the API functions more consistent. We can create all types of objects through the API, except reports and labels. Being able to change them is good, but I don't see why we shouldn't be able to create reports and labels through the API. This is one function which currently requires the ADMIN page, and there is no alternative.

The python binding won't work even with this PR, as that cannot handle file uploads. Curl works fine though.

@miggland as @matmair states above this will not actually allow you to create a report or label due to the actual template file being exposed to the API.

I hope my example above proves that this change actually achieves exactly what I want, in a very simple manner with nearly no changes. I was holding back since I thought there was a good reason this wasn't implemented earlier, but from all your comments I'm guessing the explanation is simply because no one considered in detail how this works yet (which is natural, and I'm not trying to point fingers here).

Is there a reason why this is a file and not stored in the database? In regards to #2777, #2585 storing them in the database would simplify things.

Also not relevant to this PR, as it doesn't change anything in the backend. It's an API change only.

I think that the live editor approach is the end goal here. Uploading and re-uploading to make a small change is really annoying, and the whole report templating system would be way more useful if you could see the effects of your changes live. The new platform UI should make this not too hard - we will have to change the API anyway to make this work.

Sure, but this change is low-hanging fruit, as it just requires a different base class which is there anyway. A Live-editor will probably take much longer to implement.

The labels work in a similar way to reports, I'll leave that to you to try out.

@SchrodingersGat
Copy link
Member

@miggland my mistake - I thought the we were not exposing the "template" field to the API. On review, that field is already available, as you have identified :)

My concern was that you had not added the "template" field to the API - but it is already there, so that is fine.

I agree with the intent of this PR. It should be possible to create a new template with the API. Certainly moving forward we want to be able to do this without directing users to the admin interface. And this will be required for any sort of live editing functionality.

@miggland miggland marked this pull request as ready for review November 25, 2023 12:05
@SchrodingersGat
Copy link
Member

As this changes the external API functions please increment the version number in api_version.py

@miggland
Copy link
Contributor Author

Forgot about that step.. done now.

@SchrodingersGat
Copy link
Member

Thanks @miggland and sorry for the confusion :)

@SchrodingersGat SchrodingersGat merged commit 43434fc into inventree:master Nov 26, 2023
26 checks passed
@miggland miggland deleted the reportlabelcreate branch November 26, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants