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: Twitter login notifications, incorrect cookie management. #1330

Merged
merged 5 commits into from
Dec 23, 2024

Conversation

ChristopherTrimboli
Copy link
Contributor

Relates to:

No issue.

Risks

LOW - could break logins, but I tested.

Background

I noticed multiple login notifications in twitter:

image

My previous changes were not fully correct.

What does this PR do?

Handles twitter cookies better in twitter without re-triggering login() each time.

What kind of change is this?

Bug fixes (non-breaking change which fixes an issue)

Why are we doing this? Any context or related work?

I recently pushed: #1288

Not sure if my PR broke this, or was always issue, but now fixed, not getting notifications anymore with cached cookies.

Documentation changes needed?

My changes do not require a change to the project documentation.

I cleared cache and watch the twitter login notifications in app. No longer getting notifications of "new login".

Testing

Where should a reviewer start?

Try login twice with cached cookies and should see no more new login notifications.

Detailed testing steps

None, automated tests are fine.

Discord username

cjft

@ChristopherTrimboli
Copy link
Contributor Author

I highly recommend merge this before new release, required fix, also CI is failing:

Skipping integration tests due to missing required API keys
Error: Process completed with exit code 1.```

@shakkernerd shakkernerd deleted the branch elizaOS:develop December 22, 2024 07:02
@madjin madjin reopened this Dec 22, 2024
Copy link
Contributor

@proteanx proteanx left a comment

Choose a reason for hiding this comment

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

Can confirm this bug and have tested this fix and it works.

LGTM - thanks for recognizing the issue and fixing that!

Copy link
Collaborator

@monilpat monilpat left a comment

Choose a reason for hiding this comment

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

LGTM thanks for doing this :)

@monilpat monilpat merged commit b1a6b9d into elizaOS:develop Dec 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants