Fix: Error in creditmemo table header and other small fixes

firegento / firegento-pdf

Generates nicer and configurable PDF for invoices, creditmemos and shippings in Magento

100 stars 62 forks source link

Fix: Error in creditmemo table header and other small fixes #334

Closed atiklabs closed 6 years ago

atiklabs commented 7 years ago

Creditmemo table header, for a non default order of price columns of items configuration.

Basically, in the creditmemo the table header is always the same, but the column fields are not always the same.

atiklabs commented 7 years ago

I also included a commit with a small fix when printing only one shipment pdf from grid, and with the last one commit the shipment pdf will show all the tracking numbers (if there is at least one and order is not virtual).

Schrank commented 7 years ago

If you submit it again, may you add a screenshot? I have no clue what you did :)

Schrank commented 7 years ago

And thanks for working on it!

atiklabs commented 7 years ago

In the next image you can see what was happening before the first commit. In creditmemo PDF columns in the header are not matching the columns in the body of the table.

before-commit-b26ba6c

Second commit basically fixes a small error, it was trying to print a shipment as an invoice. It's just much easier to check the commit code.

Last commit will print tracks in the shipment pdf.

print-shipment-tracks

atiklabs commented 7 years ago

Thanks to all your team for this extension!

sprankhub commented 7 years ago

Thanks for your PR and sorry for the delay! For the next time, please split your changes in separate PRs.

b26ba6c: You are right - this is a bug. But I think it is not really correct to use the same column order for invoices and creditmemos since the current configuration should only apply to invoices (Mage::getStoreConfig('sales_pdf/invoice/item_price_column_order')). I think it would be better to have the same configuration option for creditmemos and use this one here. Would you like to implement that and propose a new PR for it?
4e11ce1: Should be merged. Could you send a separate PR for this change?
af1c1b8: Not sure about this one. If we implement this functionality, I think there should be an option to disable it. Right @Schrank?

Schrank commented 7 years ago

I like https://github.com/firegento/firegento-pdf/commit/af1c1b87e10fe965af23c485436c80fc95eef889 and yes, needs to be disabled by default. And due to the fact, that we are so late, we should do the split on our own @sprankhub

atiklabs commented 7 years ago

Hello, sorry that I didn't do different PR, ok for next time. I'm sorry but right now I don't have time to work on it. Maybe in the future. Regards.

sprankhub commented 7 years ago

I just cherry-picked / merged https://github.com/firegento/firegento-pdf/commit/4e11ce14a598a3aa70514d0226de90503927b810.

sprankhub commented 6 years ago

https://github.com/firegento/firegento-pdf/commit/b26ba6cd00eecca135400cc50ac32499d4e7682f: This has already been fixed in https://github.com/firegento/firegento-pdf/commit/e4529f3c8cede52fbc7bfadaec0462ab410437dd. We use the same table header and sorting for the creditmemo and invoice for the time being. https://github.com/firegento/firegento-pdf/commit/4e11ce14a598a3aa70514d0226de90503927b810: This has already been merged. https://github.com/firegento/firegento-pdf/commit/af1c1b87e10fe965af23c485436c80fc95eef889: This has been implemented in https://github.com/firegento/firegento-pdf/commit/9cf8a75c669951fd50fa8829aab0696258c78ac7.

Thanks @atik-lab!