added functionality to create new facility on existing kolibri #11197

Jaspreet-singh-1032 · 2023-09-05T17:32:45Z

Summary

closes #10826
Added functionality to create a new facility on the existing Kolibri

Added a modal to create new facility
Implemented backend APIs to create facility
screenshots

…

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

github-actions · 2023-09-05T18:29:10Z

Build Artifacts

akolson · 2023-09-06T12:02:00Z

kolibri/plugins/device/assets/src/constants.js

+export const Presets = Object.freeze({
+  FORMAL: 'formal',
+  NONFORMAL: 'nonformal',
+});


Hi @Jaspreet-singh-1032. There looks to be similar constants in kolibri/plugins/setup_wizard/assets/src/constants.js, so I think its better to use them instead for maintenance purposes!

We can always move this to a more central location to eliminate the deep paths(ie ../../../../../)

Hi @akolson, I have made some changes. Have a look and let me know if it needs any other changes.
Thanks.

Maybe a clean up of this? Looks like Presets has been moved to kolibri/core/assets/src/constants.js so nolonger necessary in here

pcenov

Hi @Jaspreet-singh-1032 and @rtibbles, this is implemented exactly as specified in #10826 however I can see the following potential issues with this implementation:

The user is presented with an option to choose between a non-formal and formal facility however since the rest of the steps from the setup wizard are missing the created facility is actually created with all the facility settings set to true regardless of the option I've selected. Thus the default Settings of a formal facility look like this:

For the above mentioned reason there is no admin user in the created facility so that user has to be added separately if one wants to be able to import it on another device.
The 'Add facility' button and the 'Create a new learning facility' modal are not yet localized in other languages.

akolson · 2023-09-12T13:27:42Z

kolibri/plugins/device/assets/src/views/FacilitiesPage/index.vue

+            value: 'create_new_facility',
+          },
+        ];
+      },


I think label fields should be translated. Is this a temporal measure? cc @rtibbles

It would also be good to add the value fields as constants in kolibri/plugins/device/assets/src/constants.js as they seem specific to the device plugin

Yes, the label values should be translated strings.

akolson · 2023-09-12T13:36:19Z

kolibri/plugins/device/assets/src/views/FacilitiesPage/CreateNewFacilityModal.vue

+    data() {
+      return {
+        facilityName: '',
+        preset: 'nonformal',


We probably want to now use the Presets.NONFORMAL here instead of hardcoded nonformal string.

akolson · 2023-09-12T13:41:45Z

kolibri/plugins/device/assets/src/views/FacilitiesPage/index.vue

@@ -389,6 +420,13 @@
            this.$emit('failure');
          });
      },
+      handleSelect(option) {
+        if (option.value == 'import_facility') {


See my comments on value fields above

akolson

Generally code looks good, thanks @Jaspreet-singh-1032. There's is just a few clean up tasks to do and we should be good. Its probably good to hold off any further approvals in light of @pcenov's comments.

rtibbles · 2023-09-15T18:44:57Z

For @pcenov's points:

We need to make the presets actually do something, otherwise giving this choice to the user is meaningless, so there's a bit of work to do here to ensure the presets are applied and the proper settings are handled.
I think we can stomach not creating an admin during this workflow, as the user creating this facility has to be a superadmin - so they can use the facility tooling to manage the users.
We should be internationalizing all user interface text, as noted by @akolson above as well - it seems this has now been done!

So - to get this merged, I would say we need to address 1.

rtibbles · 2023-09-15T18:46:41Z

kolibri/core/auth/serializers.py

+
+    class Meta:
+        model = Facility
+        fields = ("id", "name", "preset")


You are taking the preset here, but not doing anything with it.

See here for an example of how the preset data is used in a serializer in order to set the settings appropriately: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/device/serializers.py#L139

This would require adding a create method to this serializer to implement this custom logic.

…lity create

pcenov · 2023-09-18T07:54:53Z

Hi @Jaspreet-singh-1032 please let me know when this one is ready for another round of manual testing. Thanks!

Jaspreet-singh-1032 · 2023-09-18T13:41:13Z

Hi @pcenov, this is ready for testing.

pcenov

Confirmed - the presets for Formal and Non-formal facility are implemented correctly now. Good to go!

akolson

Thanks @Jaspreet-singh-1032! Please see some additional thoughts here, that I think would be the final piece for the merge. Unless @rtibbles has any other thoughts, we should be good after this.

akolson · 2023-09-22T10:04:42Z

kolibri/core/auth/serializers.py

+            )
+            facility.dataset.reset_to_default_settings(validated_data.get("preset"))
+        return facility
+


@Jaspreet-singh-1032 thanks for adding the create method. However, here are a few more comments to further make the code more robust;

You could set the preset and name fields as variables instead eg.

preset = validated_data.get("preset")

Then add some validation checks to these variables before proceeding to use them. You can alternatively use the validate method within the serializer class if you like

It's also a good idea to wrap the code around a try..catch block just in case any of the transactions fail

Hi @akolson, I've made the changes you suggested by setting preset and name fields as variables. However, I have not added additional validation to these variables because I believe the data is already being validated at this point. if there are specific scenarios or edge cases where you think further validation is needed, please let me know, and I'll be happy to implement it accordingly. And I have also added the try-catch block.

I see the name is being validated on the frontend, it's probably good to add this validation on the backend too. But, I could be lacking a bit of context overall so I am happy to proceed with the merge(Changes look good, thanks :)) as long as @rtibbles is happy with the changes.

By default the serializer will do introspection of the Django model, so that should be doing some validation to ensure it doesn't exceed the maximum length of the name attribute, and that it is a string, so I think Django REST Framework magic is probably doing the leg work for us here!

akolson

Great work @Jaspreet-singh-1032!

rtibbles · 2023-09-25T14:55:23Z

kolibri/core/auth/api.py

@@ -621,6 +622,13 @@ def annotate_queryset(self, queryset):
            )
        )

+    @decorators.action(methods=["post"], detail=False)
+    def create_facility(self, request):
+        serializer = CreateFacilitySerializer(data=request.data)


I think the only thing that this is missing is a permissions check here - we should require that request.user.is_superuser is True otherwise, we should raise a permissions error before doing anything else here.

Excellent - yes using the permission classes in the decorator is a very neat way to do this!

akolson

Adding a little blocker here in light of Richards comment on adding a permission check to the serializer. . We should be good for a merge once this check added.

Changes addressed!

rtibbles · 2023-09-25T23:41:35Z

Build breakage is caused by erroneous merge to develop, not by this PR. Merging! Great work @Jaspreet-singh-1032, this is really nicely done!

Jaspreet-singh-1032 · 2023-09-26T15:18:15Z

Thanks @pcenov @akolson and @rtibbles for your support during this PR :)

added functionality to create new facility on existing kolibri

c85df81

github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: medium labels Sep 5, 2023

MisRob requested review from pcenov, rtibbles and akolson September 6, 2023 10:52

akolson reviewed Sep 6, 2023

View reviewed changes

moved Presets constant to core assets constants

b39eac9

github-actions bot added the APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) label Sep 7, 2023

Jaspreet-singh-1032 requested a review from akolson September 7, 2023 17:40

pcenov reviewed Sep 12, 2023

View reviewed changes

akolson reviewed Sep 12, 2023

View reviewed changes

akolson requested changes Sep 12, 2023

View reviewed changes

code cleanup

6d36f25

rtibbles reviewed Sep 15, 2023

View reviewed changes

apply facility settings based on preset and show notification on faci…

bced591

…lity create

pcenov self-requested a review September 19, 2023 08:10

pcenov approved these changes Sep 19, 2023

View reviewed changes

Jaspreet-singh-1032 requested a review from akolson September 20, 2023 17:32

akolson requested changes Sep 22, 2023

View reviewed changes

akolson reviewed Sep 22, 2023

View reviewed changes

set name and preset as variables and added try except

df5bdb1

Jaspreet-singh-1032 requested a review from akolson September 24, 2023 18:42

akolson approved these changes Sep 25, 2023

View reviewed changes

rtibbles reviewed Sep 25, 2023

View reviewed changes

akolson previously requested changes Sep 25, 2023

View reviewed changes

added is_superuser permission check before creating facility

00a6031

rtibbles approved these changes Sep 25, 2023

View reviewed changes

rtibbles merged commit 4b7239b into learningequality:develop Sep 25, 2023
33 of 34 checks passed

rtibbles mentioned this pull request Sep 25, 2023

Make it possible to create a new facility on existing Kolibri #10826

Closed

Jaspreet-singh-1032 mentioned this pull request Sep 26, 2023

added test cases for create new facility feature #11303

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added functionality to create new facility on existing kolibri #11197

added functionality to create new facility on existing kolibri #11197

Jaspreet-singh-1032 commented Sep 5, 2023

github-actions bot commented Sep 5, 2023 •

edited

Loading

akolson Sep 6, 2023

akolson Sep 6, 2023

Jaspreet-singh-1032 Sep 9, 2023

akolson Sep 12, 2023

pcenov left a comment

akolson Sep 12, 2023 •

edited

Loading

akolson Sep 12, 2023

rtibbles Sep 13, 2023

akolson Sep 12, 2023

akolson Sep 12, 2023 •

edited

Loading

akolson left a comment •

edited

Loading

rtibbles commented Sep 15, 2023

rtibbles Sep 15, 2023

pcenov commented Sep 18, 2023

Jaspreet-singh-1032 commented Sep 18, 2023

pcenov left a comment

akolson left a comment

akolson Sep 22, 2023 •

edited

Loading

Jaspreet-singh-1032 Sep 24, 2023

akolson Sep 25, 2023

rtibbles Sep 25, 2023

akolson left a comment

rtibbles Sep 25, 2023

rtibbles Sep 25, 2023

akolson left a comment

rtibbles commented Sep 25, 2023

Jaspreet-singh-1032 commented Sep 26, 2023

added functionality to create new facility on existing kolibri #11197

added functionality to create new facility on existing kolibri #11197

Conversation

Jaspreet-singh-1032 commented Sep 5, 2023

Summary

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcenov left a comment

Choose a reason for hiding this comment

akolson Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

akolson left a comment • edited Loading

Choose a reason for hiding this comment

rtibbles commented Sep 15, 2023

Choose a reason for hiding this comment

pcenov commented Sep 18, 2023

Jaspreet-singh-1032 commented Sep 18, 2023

pcenov left a comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

akolson Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

rtibbles commented Sep 25, 2023

Jaspreet-singh-1032 commented Sep 26, 2023

github-actions bot commented Sep 5, 2023 •

edited

Loading

akolson Sep 12, 2023 •

edited

Loading

akolson Sep 12, 2023 •

edited

Loading

akolson left a comment •

edited

Loading

akolson Sep 22, 2023 •

edited

Loading