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

Prevent remote teams from overwriting local teams #2163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Karthik99999
Copy link
Contributor

The extra loaded check seems to be allowing local teams to be overwritten with remote team data on every refresh, which doesn't seem to be the intended behavior given this comment in storage.js.

// prioritize locally saved teams over remote
// as so to not overwrite changes

@DaWoblefet DaWoblefet requested a review from mia-pi-git October 16, 2023 13:22
@mia-pi-git
Copy link
Member

This check is necessary to ensure remote teams actually get loaded. teamid only exists on the team if it's been remotely loaded at one point or another.

Copy link
Member

@mia-pi-git mia-pi-git left a comment

Choose a reason for hiding this comment

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

This won't fix the issue described.

@Karthik99999
Copy link
Contributor Author

This check is necessary to ensure remote teams actually get loaded. teamid only exists on the team if it's been remotely loaded at one point or another.

I tested the change on the testclient and the team loaded in just fine initially. Is there an edge case that I missed?

@mia-pi-git
Copy link
Member

mia-pi-git commented Oct 16, 2023

it works fine with actual remote teams, the problem is non-remote (local-only) teams, which it tries to load forever without this check. That's why I added it in the first place (the === false one should probably just be removed as is.)

@Karthik99999
Copy link
Contributor Author

I'm a bit confused on this. My local only teams seemed to work just fine as well, and wouldn't local only teams not have a teamid anyway?

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.

2 participants