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

[IP-1012]: Revert change to invoice_logo() #1013

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

jmclaren7
Copy link
Contributor

@jmclaren7 jmclaren7 commented Jan 5, 2024

invoice_logo() produces an invalid URL based on an absolute file system path, it seems this change was only intended for invoice_logo_pdf()

Related issue: #1012

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

application/helpers/invoice_helper.php Outdated Show resolved Hide resolved
@nielsdrost7 nielsdrost7 changed the title Revert change to invoice_logo() [IP-1012]: Revert change to invoice_logo() Jan 5, 2024
@nielsdrost7 nielsdrost7 linked an issue Jan 5, 2024 that may be closed by this pull request
@jmclaren7 jmclaren7 self-assigned this Jan 18, 2024
Copy link

@Computer-Ron Computer-Ron left a comment

Choose a reason for hiding this comment

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

I am a novice interacting with GitHub so apologize in advance if not following proper procedure and/or protocol. I have implemented this change on my 1.6.1 installation and can report this change indeed resolves the logo not showing on email invoices issue by using the correct path for finding the file on the server (base vs absolute).

@jmclaren7
Copy link
Contributor Author

Thanks @Computer-Ron for confirming you had a good result with the change, someone with access to the repository needs to make the review for it to automatically merge but maybe your input will prompt @nielsdrost7 to lend a hand, thanks!

@nielsdrost7 nielsdrost7 merged commit aa1989c into InvoicePlane:development Apr 22, 2024
1 check passed
@nielsdrost7
Copy link
Contributor

@jmclaren7 merged!

@Computer-Ron thanks for the PR, man! Keep'em coming

Copy link

@jmmeijer jmmeijer left a comment

Choose a reason for hiding this comment

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

This is correct, thanks

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

Successfully merging this pull request may close these issues.

Web invoice does not show logo
4 participants