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

Ensure peer import and sync tasks for data and content work with servers using a prefix path #9533

Merged
merged 8 commits into from Jul 12, 2022

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 23, 2022

Summary

  • Includes the server prefix in zeroconf broadcast information
  • Adds a prefix to the KolibriInstance baseurl
  • Fixes url concatenation to ensure that prefixes persist
  • Prevents magicbus from throwing an uncaught exception when an exception is caught
  • Maintain prefix on NetworkClient when it has successfully detected the actual URL for a server

References

Fixes #9455

Reviewer guidance

  • Run one server with either the Deployment URL_PATH_PREFIX option set in options.ini or with the env var KOLIBRI_PATH_URL_PREFIX to so something other than /.
  • Run another server unprefixed.
  • Import a facility to the unprefixed server from the prefixed server.
  • Import resources to the unprefixed server from the prefixed server.
  • Sync a facility on the unprefixed server with the prefixed server.

For regression testing, worth testing these scenarios without any prefixing to ensure everything is working.

I have done manual testing of the three scenarios above, and ensured that resource import still functions for unprefixed servers.

One caveat is that our zeroconf info does not update well when a server changes its baseurl - not sure if there's something that can be done about that?


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jun 23, 2022
@rtibbles rtibbles added this to the Planned Patch 4: Coach usability improvements milestone Jun 23, 2022
@pcenov
Copy link
Member

pcenov commented Jun 30, 2022

@rtibbles
Tested the above mentioned scenarios in Windows, Linux and Android and all are passing. Also regression tested all main workflows without prefixing.
The only issue I found is that assigned quizzes cannot be viewed and I'm getting a "Resource not found" error. This is happening with both enabled/disabled URL_PATH_PREFIX option set in options.ini:

2022-06-30_16-36-42

I also tried importing a facility from a prefixed server to another prefixed server but it says that there's no such user when I enter the admin credentials so I'm guessing this is not a valid scenario:

2022-06-30_17-27-52

Logs and DB:
UbuntuLogsAndDB.zip

@rtibbles
Copy link
Member Author

Hrm, I can't think why anything I have done would cause the first issue. I assume this is not extant in release-v0.15.x?

The prefix to prefix issue should be valid - I am pretty sure I know why this is happening too, can fix up.

@pcenov
Copy link
Member

pcenov commented Jun 30, 2022

Indeed I just checked the official 0.15.4 build and have filed a separate issue for that: #9544

@rtibbles
Copy link
Member Author

rtibbles commented Jul 5, 2022

@pcenov - this should now be updated to properly work when both servers are prefixed.

@pcenov
Copy link
Member

pcenov commented Jul 6, 2022

@rtibbles I confirm that now the import is working with both servers prefixed but it's no longer working when both are not prefixed and also when I attempt to import a facility from a prefixed server to an unprefixed one.
LogsDB.zip

@rtibbles
Copy link
Member Author

rtibbles commented Jul 6, 2022

Goddammit!

@rtibbles
Copy link
Member Author

rtibbles commented Jul 6, 2022

Thanks @pcenov - I have now tested locally with the 2x2 matrix of (prefix, not-prefixed) with (prefix, not-prefixed) - and all scenarios do now seem to be working. But I'd very much appreciate confirmation!

@pcenov
Copy link
Member

pcenov commented Jul 7, 2022

@rtibbles - I also tested all possible combinations plus regression tested the normal unprefixed workflows and everything is functioning correctly now.
Unfortunately I'm afraid that there is one more case to handle: if I attempt to setup a learn-only device by importing a learner from a prefixed server I'm not able to do so as I'm getting the same error for non-existing user as previously reported (while the user is perfectly valid):

2022-07-07_15-48-11

@rtibbles
Copy link
Member Author

rtibbles commented Jul 7, 2022

Ahah! Thanks @pcenov looks like there's one more code path I need to fix.

@rtibbles
Copy link
Member Author

rtibbles commented Jul 7, 2022

To try to definitively resolve this, I have grepped the codebase for all instances of reverse that seem to be then subsequently passed to urljoin for some sort of remote URL - hopefully I have now caught any remaining instances!

Would be good to validate full single user import and then automatic syncing flow.

@pcenov
Copy link
Member

pcenov commented Jul 8, 2022

Double checked and tested today again the Android learn-only device setup from both prefixed/unprefixed server, resource import and syncing - everything is working correctly now.
Regression tested device import and syncing and channel import from both prefixed/unprefixed server as well as prefixed to prefixed and unprefixed to unprefixed server between Windows and Ubuntu devices... LGTM! :)

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

To the great disappointment of @pcenov he did not manage to find any other issues here! 😂 😈

Ready to merge!!! 💯 :shipit:

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

If this is thought to be backward compatible, it's not working correctly when the unprefixed server is runinng 0.15.3. If that's not the goal, it's all right: servers running this code work fine both uprefixed or prefixed.
There are no regressions either: when unprefixed it works correctly with previous versions of kolibri.

@rtibbles
Copy link
Member Author

it's not working correctly when the unprefixed server is runinng 0.15.3

If this means that a server running 0.15.3 cannot import from a prefixed server (even if it's running this) then that is expected behaviour. If this means that a server running this version cannot import from an unprefixed 0.15.3, that's an issue. Which is it?

@jredrejo
Copy link
Member

it's not working correctly when the unprefixed server is runinng 0.15.3

If this means that a server running 0.15.3 cannot import from a prefixed server (even if it's running this) then that is expected behaviour. If this means that a server running this version cannot import from an unprefixed 0.15.3, that's an issue. Which is it?

the first, a server running 0.15.3 can not import from a prefixed server. The second option worked perfectly

@rtibbles
Copy link
Member Author

OK, good - this is the behaviour that this PR is fixing! So it seems this is working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
4 participants