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

Update /users/creator_node route to include non-creator users #1291

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

SidSethi
Copy link
Contributor

@SidSethi SidSethi commented Mar 10, 2021

Description

What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?

I found this bug because it broke SnapbackSM, which relies on this route. Specifically, all recurring syncs for non-creator users were broken. This isn't a p0 because majority of users syncs are going through manual sync flow, but still no bueno.

Discprov GET /users/creator_node?creator_node_endpoint=<url> route was not returning data for users that had replica sets but were not creators. Previously this intersection was impossible as we only assigned replica sets after flipping is_creator flag, but recently we changed this in order to assign replica sets to all users on signup to ensure replication of content for all non-creator users.
This bugfix simply replaces the is_creator check in this route with a creator_node_endpoint IS NOT NULL check.

There are similar regressions across the codebase, and separately we need to do comb through and update all references to is_creator.

Tests

List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗

Tested this locally with bugfix, confirming that non-creator users are also returned by this endoint.

❗ Reminder 💡❗:
If this PR touches a critical flow (such as Indexing, Uploads, Gateway or the Filesystem), make sure to add the requires-special-attention label. Add relevant labels as necessary.

@SidSethi SidSethi added bug Something isn't working discovery-node Discovery Node (previously known as Discovery Provider) labels Mar 10, 2021
@vicky-g
Copy link
Contributor

vicky-g commented Mar 10, 2021

good find!

Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

LGTM, huge find 💯

@SidSethi SidSethi merged commit 11b2772 into master Mar 10, 2021
@SidSethi SidSethi deleted the ss-discprov-users-by-cn-bugfix branch March 10, 2021 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discovery-node Discovery Node (previously known as Discovery Provider)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants