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

Improvements for the due date feature #641

Merged
merged 15 commits into from
Nov 8, 2023
Merged

Improvements for the due date feature #641

merged 15 commits into from
Nov 8, 2023

Conversation

MohamadNateqi
Copy link
Contributor

close #640

Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

Just one remark. You have placed the method get_due_date() in the Invoice document class, but the display_due_date() on the main class. Since this is restricted to the Invoice, don't you think they should both be in the Invoice class?

@MohamadNateqi
Copy link
Contributor Author

It seems that the invoice class is being called multiple times. If we were to move the display_due_date() function to this class, it would add a due date to the invoice every time it's called.
Initially, I tried this approach, but I noticed that multiple due dates were being added to the invoice, which made me realize that the class was being called multiple times. So, I moved the function back to the main class.

@alexmigf alexmigf self-requested a review November 6, 2023 09:02
Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

In that case I think we should make the function display_due_date a bit more flexible:

  • adding a filter in the document type check
  • using wcpdf_get_document instead of wcpdf_get_invoice
  • add a callable check under get_due_date, just in case
  • Renaming this hook: wpo_wcpdf_{document_slug}_due_date_display

@MohamadNateqi
Copy link
Contributor Author

In that case I think we should make the function display_due_date a bit more flexible:

To make it more general purpose instead of just working for the invoice document. Got it.

includes/documents/class-wcpdf-invoice.php Outdated Show resolved Hide resolved
includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
includes/documents/class-wcpdf-invoice.php Outdated Show resolved Hide resolved
includes/documents/class-wcpdf-invoice.php Outdated Show resolved Hide resolved
includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
@MohamadNateqi MohamadNateqi changed the title Add label to due date Improvements for the due date feature Nov 7, 2023
@MohamadNateqi
Copy link
Contributor Author

@alexmigf In the latest commit, I also updated the hook name for filters. Instead of adding the document slug in the hook name, I send the document type as an argument to make them uniform. It seemed there was no need to have different hook names for them.
Please let me know if it's not a good idea, and I'll revert that.

@MohamadNateqi
Copy link
Contributor Author

@alexmigf Ready to review.

includes/documents/abstract-wcpdf-order-document.php Outdated Show resolved Hide resolved
includes/documents/class-wcpdf-invoice.php Outdated Show resolved Hide resolved
includes/documents/class-wcpdf-invoice.php Show resolved Hide resolved
includes/documents/class-wcpdf-invoice.php Outdated Show resolved Hide resolved
includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
includes/documents/abstract-wcpdf-order-document.php Outdated Show resolved Hide resolved
includes/documents/abstract-wcpdf-order-document.php Outdated Show resolved Hide resolved
includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
@alexmigf alexmigf self-requested a review November 8, 2023 09:03
Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

Perfect!

@alexmigf alexmigf merged commit af3ba05 into main Nov 8, 2023
5 checks passed
@alexmigf alexmigf deleted the 640-due_date_label branch November 8, 2023 09:08
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.

Improvements for the due date feature
2 participants