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

ui: Coordinates don't require a nspace, so don't expect one in the repo #7378

Merged
merged 2 commits into from
Mar 4, 2020

Conversation

johncowen
Copy link
Contributor

This PR ensures that the index blocking query parameter/cursor is maintained whilst blocking on the coordinates endpoint.

@johncowen johncowen added type/bug Feature does not function as expected theme/ui Anything related to the UI labels Mar 3, 2020
@johncowen johncowen requested a review from a team March 3, 2020 17:02
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

♻️ LGTM

service.findAllByDatacenter = function(dc, configuration) {
assert.equal(arguments.length, 2, 'Expected to be called with the correct number of arguments');
assert.equal(dc, datacenter);
assert.deepEqual(configuration, conf);
Copy link
Contributor

@kaxcode kaxcode Mar 3, 2020

Choose a reason for hiding this comment

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

configuration is not set here so it equals {}. How does it equal cursor: 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember we are completely replacing the findAllByDatacenter method here. findByNode calls our fake findAllByDatacenter with the dc and configuration arguments, which it gets from the original call to findAllbyNode('node-name', datacenter, conf), conf being {cursor: 1}, so eventually configuration === {cursor: 1}

Previously we were expecting and passing through an extra nspace argument, which was actually the configuration object, which was the root of the bug.

@johncowen johncowen merged commit 6074a1b into master Mar 4, 2020
@johncowen johncowen deleted the ui/bugfix/coordinate-blocking-queries branch March 4, 2020 18:12
@johncowen johncowen added this to the 1.7.2 milestone Mar 12, 2020
freddygv pushed a commit that referenced this pull request Mar 12, 2020
…po (#7378)

* ui: Coordinates don't require a nspace, so don't expect one in the repo

* Add a test to prove the correct number of arguments are being passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants