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

Fix: issues rendering the number tab results #755

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

alexmigf
Copy link
Member

@alexmigf alexmigf commented Mar 21, 2024

closes #752

This pull request aims to enhance the performance of the number tables query by caching the data in an option. This approach ensures that the data can be reused in subsequent requests, thus reducing the load on the store.

Additionally, I've introduced a new selector that allows for limiting the query results. This is particularly useful if there's a need to retrieve, for instance, only the most recent 100 numbers.

The implementation also includes functionality to track and display the last fetch date. This feature is instrumental in determining whether there's a need for a fresh fetch of data.

The cached data is designed to be deletable, ensuring flexibility and independence across different tables.

Furthermore, I've made minor stylistic adjustments to the table, especially within the type column. For unassigned numbers, the column now displays gapped. In the context of credit-note tables, it also shows the refund order ID, enhancing clarity and information accessibility.

The functionality for number filtering and search has been preserved.

Screenshot from 2024-03-21 16-20-13

assets/js/debug-script.js Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
Copy link
Contributor

@BrunoPavlinic98 BrunoPavlinic98 left a comment

Choose a reason for hiding this comment

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

LGTM!

includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/settings/class-wcpdf-settings-debug.php Outdated Show resolved Hide resolved
includes/tables/class-wcpdf-number-store-list-table.php Outdated Show resolved Hide resolved
Copy link
Contributor

@MohamadNateqi MohamadNateqi left a comment

Choose a reason for hiding this comment

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

LGTM!

@dwalkerpriv
Copy link
Contributor

The update seems to be working so far. I've noticed these documents in the drop down. Did we add these in the update?

Screenshot 2024-03-27 134725

@alexmigf
Copy link
Member Author

@dwalkerpriv no, those are tables that were created in your local setup, probably on tests you made for customers, for example using code snippets.

@alexmigf alexmigf added this to the 3.8.1 milestone Apr 4, 2024
Copy link
Contributor

@Terminator-Barbapapa Terminator-Barbapapa left a comment

Choose a reason for hiding this comment

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

I'm not sure if using AS is the best way of doing this. As the page has to be reloaded to see actual results, which is not a good UX.

includes/tables/class-wcpdf-number-store-list-table.php Outdated Show resolved Hide resolved
@alexmigf
Copy link
Member Author

I'm not sure if using AS is the best way of doing this. As the page has to be reloaded to see actual results, which is not a good UX.

What alternatives would you recommend? I believe that in large stores, waiting for page data can also be a lengthy and unpleasant experience.

@Terminator-Barbapapa Terminator-Barbapapa modified the milestones: 3.8.1, 3.8.2 Apr 19, 2024
@alexmigf alexmigf modified the milestones: 3.8.2, 3.8.3 Apr 29, 2024
@alexmigf alexmigf modified the milestones: 3.8.3, 3.8.4 May 17, 2024
@dpeyou
Copy link
Contributor

dpeyou commented May 29, 2024

@alexmigf

  • I am not getting the textbox to limit the fetch.
    2024-05-29_03-45

  • The time of the fetch is in UTC, but without indication it is UTC time. That could be a source of confusion.
    2024-05-29_03-40

@alexmigf
Copy link
Member Author

@dpeyou

I am not getting the textbox to limit the fetch.

It has been replaced with the date interval selectors that you now see.

The time of the fetch is in UTC, but without indication it is UTC time. That could be a source of confusion.

The date_i18n function is being used without specifying the GMT timezone. In this case, it defaults to using the WordPress site's timezone settings.

Copy link
Contributor

@dpeyou dpeyou left a comment

Choose a reason for hiding this comment

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

From a functional standpoint, I cannot find an issue 😄

Copy link
Contributor

@YordanSoares YordanSoares left a comment

Choose a reason for hiding this comment

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

LGTM! The query worked as expected, although I'm agree with Michael that would be nice if the result could be displayed without the need to reload the plage. That said, I also think that this could be improved later :)

@alexmigf
Copy link
Member Author

alexmigf commented Jul 8, 2024

@YordanSoares

...although I'm agree with Michael that would be nice if the result could be displayed without the need to reload the page.

I understand. We've handled similar implementations before, like with the Bulk Export feature. The new code uses the Action Scheduler to efficiently manage stores with a large number of orders. This method was chosen to avoid the drawbacks of using AJAX, particularly in larger stores. With AJAX, users would have to wait, and if they closed the tab, the script would stop running, leading to a poor user experience. By using the Action Scheduler, users can leave the page and come back later to view the results if the process takes too long.

@Terminator-Barbapapa Terminator-Barbapapa modified the milestones: IPS Free: 3.8.5, IPS Free: 3.9.1 Jul 11, 2024
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.

Numbers tab exhausting memory limit
7 participants