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/923 reduce the number of api #936

Merged
merged 12 commits into from
Dec 19, 2024
Merged

Conversation

Khadreal
Copy link
Contributor

@Khadreal Khadreal commented Dec 9, 2024

Description

Fixes #923
Reduce the number of times api is called.

Type of change

  • Bug fix (non-breaking change which fixes an issue).

Detailed scenario

Check #923 for detailed scenario

Technical description

Documentation

Remove the api call from constructor and only call when it's needed.

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Unticked items justification

If some mandatory items are not relevant, explain why in this section.

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • When possible, I prepared ways to observe the implemented system (logs, data, etc.).
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

@Khadreal Khadreal marked this pull request as ready for review December 10, 2024 11:01
@Khadreal Khadreal requested a review from a team December 10, 2024 11:39
@Khadreal Khadreal self-assigned this Dec 10, 2024
@jeawhanlee
Copy link
Contributor

Good Job Sir!
This PR has only satisfied one part of the main issue, which is optimizing the way we call the API, however it does not cater for only loading those data on the bulk and settings screen as well as when the admin bar is hovered. See more details in the issue and AC here

@remyperona remyperona added this to the 2.2.4 milestone Dec 10, 2024
@Mai-Saad
Copy link

@Mai-Saad
Copy link

Mai-Saad commented Dec 18, 2024

We have one test is failing (Note: in general, sometimes if other transients than imagify_user_cache is expired while we are doing test , API call may happen but after refresh no call) => failed test acceptable by @piotrbak
testrail-report-668.pdf

@Mai-Saad Mai-Saad added this pull request to the merge queue Dec 19, 2024
Merged via the queue into develop with commit 1ab429c Dec 19, 2024
9 checks passed
@Mai-Saad Mai-Saad deleted the fix/923-reduce-the-number-of-api branch December 19, 2024 07:56
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.

Reduce the number of calls to Imagify API users/me
4 participants