Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

Added new functionality to allow LaTeX generation of invoices. #875

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

neerdoc
Copy link

@neerdoc neerdoc commented Dec 7, 2016

Changes proposed in this pull request:

  • Added functionality for LaTeX invoices.

Reason for this pull request:

  • LaTeX is one of the most complete type setting tools there are with huge community support.

} elseif (file_exists($tplFile.'/invoice.tex')) {
$extension = 'LaTeX';
//Test if we can execute pdflatex
$output = exec($kga['LaTeXExec']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you use is_executable($kga['LaTeXExec'])?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about that function. I can use that instead if you like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please.

@kevinpapst
Copy link
Member

Hi @neerdragon!
First of all: Thanks a lot for contributing such an interesting feature 👍 I will test it out within the next days.

One thing I would request is check(s) for the existence of the exec function, as it is deactivated in many environments (shared hosts for example) for security reasons:

if(function_exists('exec')) {
    // deactivate latex functionality
}

I would suggest that we dislay an error message instead of the input field for the executable in the admin section and then disable it in the invoice section as well.

@neerdoc
Copy link
Author

neerdoc commented Dec 8, 2016

Hi @kevinpapst !

Ok, I will do that! It is kind of automatically deactivated in the invoice section if it cannot execute, but I'll look into changing that. What about the error message? Is it ok to simply replace the input field with a message? Or should it be more "fancy", like a floater?

One more thing, I added a field in the database to store the executable, i.e. LaTeXExec, but I didn't dare change anything in the update database scripts... So I guess I need help with that if this is approved!

@neerdoc
Copy link
Author

neerdoc commented Dec 8, 2016

Added fixes as suggested.

@simonschaufi
Copy link
Contributor

please no floater. otherwise it would open every time you go to admin panel which would be very annoying.

For the db migration: make sure the installer does contain the column and the updater adds it (and increment the version number!)

if(!function_exists('exec')) {
// deactivate latex functionality
Kimai_Logger::logfile("Cannot execute external files. LaTeX invoices will be disabled.");
$view->assign('canExecute', false);
Copy link
Contributor

@simonschaufi simonschaufi Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name it "execAvailable". What do you think? canExecute sounds more like some internal stuff (object) can execute something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I can change it!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

language/en.php Outdated
@@ -257,6 +257,8 @@
"roundTimesheetEntries" => "Round new timesheet entries to",
"minutes" => "Minutes",
"seconds" => "Seconds",
"LaTeXExecutable" => "Full path to LaTeX executable, i.e., 'pdflatex'.",
"cannotExecute" => "Your system does not allow execution of external files. LaTeX invoice generation will be disabled.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a link where latex can be downloaded from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but then it needs to be updated in case the link changes... I believe anyone who wants to use LaTeX will know where to find it. If not, a simple "google LaTeX" will give you the answer... :)

@neerdoc
Copy link
Author

neerdoc commented Dec 8, 2016

No floater. I just changed the input field to a text instead.

@simonschaufi
Copy link
Contributor

Where is the class Kimai_Invoice_LaTeXRenderer actually? did you forget to add it?

@neerdoc
Copy link
Author

neerdoc commented Dec 9, 2016

Yes, indeed I forgot to add two files... Should be all now! :/

@simonschaufi
Copy link
Contributor

simonschaufi commented Dec 9, 2016

please adjust your code to make it PSR2 compatible (StyleCI check is green). click the Details link to automatically get a patch file and apply it.

@simonschaufi
Copy link
Contributor

simonschaufi commented Dec 10, 2016

I had a deeper look into your code now, made some changes and improvements but here are some things to consider:

  • everything inside the libraries folder must be a class! Make OCR a class!
  • did you write the OCR completely by your own? if not, sources?
  • don't kill the hard drive! use as little as possible file IO.
  • why do you execute pdflatex twice???
  • please use single quotes in "normal strings"

@neerdoc
Copy link
Author

neerdoc commented Dec 10, 2016

Ok, I can change OCR to be a class.
Yes, I wrote the OCR function myself.
The LaTeX typesetting engine works that way. You always have to run it twice.

@simonschaufi
Copy link
Contributor

simonschaufi commented Dec 10, 2016

You somehow messed up the indentation. Could you please align your code properly? If you are using phpstorm, use the autoformating, that works pretty well :D

Please also make your db changes so that this PR can be merged.

//Create invoiceId
$in = time();
$invoiceID = date('y', $in) . $customer['customerID'] . date('m', $in) . date('d', $in);
require_once 'Checksum.php';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be dropped as autoloading works out of the box if the class is correctly named.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

}

/**
* @return pdf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not return anything but just output the file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to "@return null".

@neerdoc
Copy link
Author

neerdoc commented Dec 11, 2016

@simonschaufi Please double check my change to installer.php and updater.php as I feel a bit unsure that I got it right.

Copy link
Contributor

@simonschaufi simonschaufi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good so far but you forgot to raise the revision number in includes/version.php.

@@ -345,6 +345,7 @@ function quoteForSql($input) {
('roundSeconds', '0'),
('allowRoundDown', '0'),
('defaultStatusID', '1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma missing in the end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.
Fixed. And increased revision by one in version.php

@neerdoc
Copy link
Author

neerdoc commented Dec 14, 2016

@simonschaufi It's all good now?

@simonschaufi
Copy link
Contributor

I would like to test it myself before I merge it but didn't have the time yet.

@neerdoc
Copy link
Author

neerdoc commented Aug 24, 2017

@simonschaufi Any progress on this issue?

@simonschaufi
Copy link
Contributor

simonschaufi commented Aug 24, 2017

Thank you for your ping!
Could you please also write some documentation how to setup a system (for all the noobs who don't know how to setup latex)? Please do that in the documentation repository.

Just for me, is this the minimal setup for ubuntu?

sudo apt-get install texlive texlive-lang-german texlive-latex-extra

@simonschaufi simonschaufi self-assigned this Aug 24, 2017
@neerdoc
Copy link
Author

neerdoc commented Aug 24, 2017

Yes, I can add some documentation!
I'm running on FreeBSD, don't know about Ubuntu... But it should be enough to install any texlive distribution.

@AaronFeldman
Copy link

AaronFeldman commented Feb 17, 2018

I would love to test this out on ubuntu 17.10!! Also, I can help proof the documentation if needed. On Ubuntu it has (at least in the past) been a bad idea to use apt-get to install texlive, because the ubuntu packages are not updated often enough, at times years behind the current distribution. The best way is to use the net installer, as described here: https://tex.stackexchange.com/a/1094/90087

@dwagenk
Copy link

dwagenk commented Aug 28, 2019

Hey,

this is the only PR or issue related to LaTeX for templates. But it looks like it's discontinued. Any chance for merging?

I'm evaluating kimai for tracking the time I spend on various freelance projects. I've got my invoices, letter-templates and basically everything else in LaTeX, so autogenerating those from within kimai would be great!

I'm not a php/web developer, but experienced enough with self hosting things like this, so I could test changes.
Got a Debian server available, but would usally configure and spin it up inside docker containers.

Thanks in advance!
Daniel

@simonschaufi
Copy link
Contributor

@dwagenk There are some conflicts with this branch as you can see so I can't just merge it like that. You can also checkout the source of the fork and see if that works for you. I don't want to waist too much time with Latex as you are the second person who is interested in it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants