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

Copies & renames useSearch to useBaseSearch in kolibri-common package (On develop this time) #12566

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Aug 16, 2024

Summary

Closes #12516

I've tried to make these changes in a way where, when @AlexVelezLl rebases on it, it should be super clear where he should be keeping his work vs the conflicting changes. Please lemme know if you hit any issues there.

References

Closes #12517

Reviewer guidance

  • Do the changes look okay for laying the groundwork?

Testing checklist

  • Contributor has fully tested the PR manually

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

I think two changes should ensure that the tests still run - but good to verify that they are still passing once these two changes have been made.


/*
* TBD #12517
const { fetchContentNodeProgress } = useContentNodeProgress();
Copy link
Member

Choose a reason for hiding this comment

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

This has been commented out, but is still referenced below. Can just implement a dummy function for now in addition to commenting this out:

const fetchContentNodeProgress = () => {};

* TBD #12517
const { baseurl } = currentDeviceData(store);
*/

Copy link
Member

Choose a reason for hiding this comment

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

Probably best to define:

const baseurl = '';

to ensure that references to it below are defined?

@nucleogenesis nucleogenesis requested a review from rtibbles August 21, 2024 19:00
@nucleogenesis nucleogenesis marked this pull request as ready for review August 21, 2024 19:00
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

I misled you with my suggestion, undefined for baseurl should give us fully working tests.

Also, this edit in develop should be propagated here: https://github.com/learningequality/kolibri/pull/12573/files#diff-97998deaa1014b5d4a58ef1c85a053b09150ff10e17ef4daf78316221eecd67dR9 (best to do a rebase to confirm things are working as they should).

const labels = ref(null);

// TODO: Kolibri #12517 - Will be injected in subsequent work
const baseurl = '';
Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, the baseurl falsy checking is actually strictly checking for undefined so setting this to:

const baseurl = undefined;

should allow all the tests to pass as expected.

});
// TODO: #12517 -- Once the injection & imports are working we should expect these tests to pass
// and should no longer be skipped
describe.skip('search method', () => {
Copy link
Member

Choose a reason for hiding this comment

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

With the change below, this skip can be removed.

@nucleogenesis nucleogenesis force-pushed the move-use-search-to-kolibri-common-on-develop branch from 877d434 to 9323112 Compare August 21, 2024 21:33
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

All comments addressed, good to merge once tests pass.

];

// TBD #12517 - Will be injected in subsequent work.
const fetchContentNodeProgress = Promise.resolve({});
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs to be a function, but we can leave that for #12517 as it seems that tests are not checking this!

@rtibbles rtibbles merged commit 30f3603 into learningequality:develop Aug 21, 2024
34 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
2 participants