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

Don't show rate limits if the reset time has passed. #356

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

iamdharmesh
Copy link
Member

Description of the Change

In #354, we added an API rate limits widget to display the remaining API rate limits for the user and app. This PR makes a minor improvement by not showing the rate limits if the reset time has passed until the latest data is available.

How to test the Change

  1. Tweet using one of the connected accounts and ensure that the rate limit starts appearing in the Dashboard widget and on the post edit screen.
  2. Edit the autoshare_for_twitter_accounts option to set the reset time to a past value for one of the user accounts.
  3. Verify that the rate limit no longer appears for that account and that it displays the message: "No X/Twitter rate limit available yet. Make a post to X/Twitter first." until new limit data becomes available.

Changelog Entry

Fixed - Ensure that no rate limits are shown if the reset time has passed.

Credits

Props @iamdharmesh

Checklist:

@iamdharmesh iamdharmesh self-assigned this Feb 1, 2025
@iamdharmesh iamdharmesh added this to the 2.3.0 milestone Feb 1, 2025
@github-actions github-actions bot added the needs:code-review This requires code review. label Feb 1, 2025
@iamdharmesh iamdharmesh requested review from a team and Sidsector9 and removed request for dkotter, jeffpaul and a team February 1, 2025 08:38
if ( $reset && $reset < time() ) {
return sprintf(
'<p>%s</p>',
esc_html__( 'No X/Twitter rate limit available yet. Make a post to X/Twitter first.', 'autoshare-for-twitter' )
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as the message here goes, do we want to still refer to things as X/Twitter or should we start just using X? Not sure how we refer to things in other places right now

Copy link
Member Author

Choose a reason for hiding this comment

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

We are still using X/Twitter in most places, but I think X is now well-known enough that we can drop Twitter from all places. @jeffpaul, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For what it's worth, my opinion here is for any new strings we introduce, we should just use X and can look to update existing strings in a separate PR. I'm also fine though if we want to continue to use X/Twitter, just noting that reads a little awkwardly and the change has been in place long enough that I think anyone using this plugin would understand what just X means

Copy link
Member

Choose a reason for hiding this comment

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

Probably fine now to update to just "Autopost for X" and "X" throughout.

Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

Code changes look good here.

While not related, both E2E tests and PHPUnit tests are failing because SVN is no longer installed by default in GitHub Actions.

The E2E tests should be fixed once 10up/action-wordpress-plugin-build-zip#8 is merged and a new release is put out.

For PHPUnit tests, we'll need to fix that on our end. Doesn't have to be done as part of this PR (could be a separate PR if you want) but would be great to resolve those. Can see how we fixed this on ClassifAI for an example

@dkotter dkotter modified the milestones: 2.3.0, 2.4.0 Feb 5, 2025
@dkotter
Copy link
Collaborator

dkotter commented Feb 5, 2025

Code changes look good here.

While not related, both E2E tests and PHPUnit tests are failing because SVN is no longer installed by default in GitHub Actions.

The E2E tests should be fixed once 10up/action-wordpress-plugin-build-zip#8 is merged and a new release is put out.

For PHPUnit tests, we'll need to fix that on our end. Doesn't have to be done as part of this PR (could be a separate PR if you want) but would be great to resolve those. Can see how we fixed this on ClassifAI for an example

E2E tests are fixed now with the upstream release. I've also fixed the PHPUnit tests in #358, so should be good on both once that is merged in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
Status: Code Review
Development

Successfully merging this pull request may close these issues.

3 participants