Device settings updates

[jredrejo]

Summary

Implementation of the new device settings UI, according to the design specs

References

Closes: #9779, #9780, #9781

Reviewer guidance

This PR implements the needed model design changes in the backend and the frontend to modify them, but it does not act in the backend to change the behaviour of kolibri as stated in the Out of scope section of #9779
However, in the case of altering the storage locations, it does change the behaviour of kolibri as the model is set to use the options.ini file as single source of truth. Changes for these settings are not stored in the database (they are done using a normal Django model and drf serialization but the state is saved in this file).

So, to after testing the UI does not have any glitches. When the new settings are modified and saved, they must appear in:

• the extra_settings field of the device_devicesettings table
• For the storage locations, in the options.ini file (also, when changing this file, kolibri will restart automatically)

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

[pcenov]

Hi @jredrejo I tested the above described implementation and overall it's looking great. As suggested by you I was able to verify that no changes made through the UI are saved as this requires backend changes. However as pointed out by you, adding other storage locations is possible because these are saved in the options.ini file. I also checked the auto-download and download on metered connections values in the DB and once they are modified there, they are displayed correctly in the UI.

Here are some minor issues I was able to identify (which I can report separately if you think are not in scope):

Issue	Screenshot
When I visit the settings page for the first time and I click the 'Options' drop-down the page moves up by itself	https://user-images.githubusercontent.com/79847249/203102254-b7a3a32b-614d-4930-99f0-4ea711589ed0.mp4
:-------------------------:	:-------------------------:
I can’t change the primary storage location if there is only one location added. Perhaps the Change option should not be displayed in that case. Note that in the current version I can’t change the primary storage location even after I have added additional locations.
:-------------------------:	:-------------------------:
The Remove storage location option should not be available if there is only one storage location.
:-------------------------:	:-------------------------:
The default value of 0 GB is not displayed correctly
:-------------------------:	:-------------------------:
@radinamatic

[LianaHarris360]

Hi @pcenov thank you for pointing these out. I am taking a look at what needs to be changed and then I will make those updates.

[LianaHarris360]

@pcenov these are the current updated and fixed issues. The page moving up due to clicking the 'Options' drop-down is still being investigated.

Issue fixed	Screenshot
If there is only one path, this is the view.
:-------------------------:	:-------------------------:
If there is only one writable path, the Change option is disabled.
:-------------------------:	:-------------------------:
Remove storage location option is shown if the 'Other storage locations' section contains more than one path. This is the view if there is are multiple writable paths and no readOnly paths in the 'Other storage locations' section.
:-------------------------:	:-------------------------:
The default value of 0 GB is now displayed correctly
:-------------------------:	:-------------------------:

[pcenov]

Hi @LianaHarris360, I can see now that initially the Change option is disabled + the displaced 0 value is displayed correctly - excellent!
However I'm still facing the following issues:

1. If I have only one storage location and I open the Options drop-down in Other storage locations and I click 'Remove storage location' I still get to see the empty radio button:

2022-11-22_15-03-10.mp4

So it should be further discussed what exactly is the expected behavior in that case.

2. This is valid even if I have added an additional storage location:

3. In this build after refreshing the page all the additional storage locations are removed, the page get's back to its default state. My understanding was these are supposed to be stored in the options.ini file so why is this happening:

2022-11-22_15-11-24.mp4

I also noticed the following issue which I have missed in the first round of testing (cc @jredrejo):

1. Why is the (read-only) text being displayed on its own? I would expect it to be displayed next to a read-only storage location:

[LianaHarris360]

@pcenov

1. I am not sure how it happened, but it seems that your 'other storage locations' section contains an empty path. So the empty radio button lists that path which I am assuming is just an empty string. I have tried to add an empty string path to possibly get the same error, but the current code does not allow an empty string to be added as a path.
2. The empty path will show after adding another location because the second location is added to a list of locations, which still includes the first empty path.
3. After making changes to the storage locations, the server should restart and once it reconnects, the changes should be updated and stored in the options.ini file. In this case, when you added the location, a 400 (bad request) error was returned and it did a soft save that showed the changes on the page but did not actually store any changes. This issue might be connected to the empty string path.

A solution could be to validate the storage location paths to catch any that slipped through as empty strings. (@jredrejo)

[jredrejo]

Hi @LianaHarris360, I can see now that initially the Change option is disabled + the displaced 0 value is displayed correctly - excellent! However I'm still facing the following issues:

1. If I have only one storage location and I open the Options drop-down in Other storage locations and I click 'Remove storage location' I still get to see the empty radio button:

2022-11-22_15-03-10.mp4
So it should be further discussed what exactly is the expected behavior in that case.

2. This is valid even if I have added an additional storage location:

3. In this build after refreshing the page all the additional storage locations are removed, the page get's back to its default state. M y understanding was these are supposed to be stored in the options.ini file so why is this happening:

2022-11-22_15-11-24.mp4
I also noticed the following issue which I have missed in the first round of testing (cc @jredrejo):

1. Why is the (read-only) text being displayed on its own? I would expect it to be displayed next to a read-only storage location:

hello @pcenov I have seen this happens if the option has not any value assigned in the options.ini file. For some reason kolibri returns [ "" ] instead of [ ]. It should be fixed now

[pcenov]

Thanks @LianaHarris360 and @jredrejo the issue with the empty value is fixed now.
So the only remaining front end issue is the one with the page moving up due to clicking the 'Options' drop-down.

@jredrejo a question for you - currently when I select the option 'Add storage location' and I check the checkbox 'Make this the primary storage location' it doesn't actually make it the primary storage location, rather after the restart of the server the location is displayed under 'Other storage location'. Is this supposed to be functional in the current build or is a pending back end task?

2022-11-23_14-37-31.mp4

[pcenov]

Hi @jredrejo @LianaHarris360 - the latest reported issue here about the storage location is working correctly now.
I have filed a follow up issue specific for the Windows build only: #9880
cc @radinamatic

[radinamatic]

Manual QA by @pcenov passed! 👏🏽 💯
One follow-up issue remains to be solved for Windows

[marcellamaki]

@jredrejo @LianaHarris360 - thanks for your work on this! The code is really readable and easy to follow, and I don't have any questions in that regard. 🎉

One thing I ran into when testing was adding a storage location. I tested:

• adding a new storage location (a valid location)
• not setting it as the primary location

When I do this, the kolibri server stops and restarts (and all of the modal explaining this work as expected), but kolibri doesn't successfully reconnect to the server.

Here is what appears in my terminal - it seems to just stall out after that last log - nothing else prints or reloads, no matter how long I wait.

Is this because I'm running it on a devserver? Wondering if @pcenov experienced the same thing. (cc @radinamatic)

[pcenov]

Hi @marcellamaki - I'm not able to replicate the issue you've experienced in Ubuntu - the server gets restarted successfully. I have previously reported a similar issue for Windows though.

[marcellamaki]

Thanks, @pcenov! I'll go ahead with merging this, and can file a separate follow up issue as needed