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

added functionality to create new facility on existing kolibri #11197

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions kolibri/core/assets/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,10 @@ export const ApplicationTypes = {
KOLIBRI: 'kolibri',
STUDIO: 'studio',
};

// aliasing 'informal' to 'personal' since it's how we talk about it
export const Presets = Object.freeze({
PERSONAL: 'informal',
FORMAL: 'formal',
NONFORMAL: 'nonformal',
});
4 changes: 4 additions & 0 deletions kolibri/core/assets/src/mixins/notificationStrings.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ export default createTranslator('NotificationStrings', {
message: 'Device not removed',
context: 'Snackbar message when a device fails to be removed from he sync schedule',
},
newLearningFacilityCreated: {
message: 'New learning facility created',
context: 'Snackbar message when a new facility created',
},
// TODO move more messages into this namespace:
// - "Quiz started"
// - "Quiz Ended"
Expand Down
9 changes: 9 additions & 0 deletions kolibri/core/auth/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
from .models import Membership
from .models import Role
from .serializers import ClassroomSerializer
from .serializers import CreateFacilitySerializer
from .serializers import ExtraFieldsSerializer
from .serializers import FacilityDatasetSerializer
from .serializers import FacilitySerializer
Expand All @@ -74,6 +75,7 @@
from kolibri.core.auth.constants.demographics import NOT_SPECIFIED
from kolibri.core.auth.permissions.general import _user_is_admin_for_own_facility
from kolibri.core.auth.permissions.general import DenyAll
from kolibri.core.device.permissions import IsSuperuser
from kolibri.core.device.utils import allow_guest_access
from kolibri.core.device.utils import allow_other_browsers_to_connect
from kolibri.core.device.utils import valid_app_key_on_request
Expand Down Expand Up @@ -621,6 +623,13 @@ def annotate_queryset(self, queryset):
)
)

@decorators.action(methods=["post"], detail=False, permission_classes=[IsSuperuser])
def create_facility(self, request):
serializer = CreateFacilitySerializer(data=request.data)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

serializer.is_valid(raise_exception=True)
serializer.save()
return Response()


class PublicFacilityViewSet(viewsets.ReadOnlyModelViewSet):
queryset = Facility.objects.all()
Expand Down
29 changes: 29 additions & 0 deletions kolibri/core/auth/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@
from __future__ import print_function
from __future__ import unicode_literals

import logging

from django.core.validators import MinLengthValidator
from django.db import transaction
from rest_framework import serializers
from rest_framework.exceptions import ParseError
from rest_framework.validators import UniqueTogetherValidator

from .constants import facility_presets
from .errors import IncompatibleDeviceSettingError
from .errors import InvalidCollectionHierarchy
from .errors import InvalidMembershipError
Expand All @@ -20,6 +25,9 @@
from kolibri.core.auth.constants.demographics import NOT_SPECIFIED


logger = logging.getLogger(__name__)


class RoleSerializer(serializers.ModelSerializer):
class Meta:
model = Role
Expand Down Expand Up @@ -137,6 +145,27 @@ class Meta:
fields = ("id", "name")


class CreateFacilitySerializer(serializers.ModelSerializer):
preset = serializers.ChoiceField(choices=facility_presets.choices)

class Meta:
model = Facility
fields = ("id", "name", "preset")
Copy link
Member

Choose a reason for hiding this comment

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

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.


def create(self, validated_data):
preset = validated_data.get("preset")
name = validated_data.get("name")
with transaction.atomic():
try:
facility_dataset = FacilityDataset.objects.create(preset=preset)
facility = Facility.objects.create(name=name, dataset=facility_dataset)
facility.dataset.reset_to_default_settings(preset)
except Exception as e:
logger.error("Error occured while creating facility: %s", str(e))
raise ParseError("Error occured while creating facility")
return facility

Copy link
Member

@akolson akolson Sep 22, 2023

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!


class PublicFacilitySerializer(serializers.ModelSerializer):
learner_can_login_with_no_password = serializers.SerializerMethodField()
learner_can_sign_up = serializers.SerializerMethodField()
Expand Down
4 changes: 4 additions & 0 deletions kolibri/plugins/device/assets/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,7 @@ export const MeteredConnectionDownloadOptions = {
DISALLOW_DOWNLOAD_ON_METERED_CONNECTION: 'DISALLOW_DOWNLOAD_ON_METERED_CONNECTION',
ALLOW_DOWNLOAD_ON_METERED_CONNECTION: 'ALLOW_DOWNLOAD_ON_METERED_CONNECTION',
};

export const ImportFacility = 'import_facility';

export const CreateNewFacility = 'create_new_facility';
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<template>

<KModal
:title="$tr('createFacilityLabel')"
:submitText="$tr('createFacilityButtonLabel')"
:cancelText="coreString('closeAction')"
size="medium"
@submit="handleSubmit"
@cancel="$emit('cancel')"
>
<p>{{ coreString('learningFacilityDescription') }}</p>
<KTextbox
ref="facilityNameTextBox"
v-model="facilityName"
:label="$tr('facilityNameFieldLabel')"
:invalid="facilityNameInvalid"
:invalidText="coreString('requiredFieldError')"
:maxlength="50"
/>
<b>{{ $tr('learningEnvironmentHeader') }}</b>
<KRadioButton
v-model="preset"
class="permission-preset-radio-button"
:value="Presets.NONFORMAL"
:label="$tr('nonFormalLabel')"
:description="$tr('nonFormalDescription')"
/>
<KRadioButton
v-model="preset"
class="permission-preset-radio-button"
:value="Presets.FORMAL"
:label="$tr('formalLabel')"
:description="$tr('formalDescription')"
/>
</KModal>

</template>


<script>

import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
import commonSyncElements from 'kolibri.coreVue.mixins.commonSyncElements';
import { Presets } from 'kolibri.coreVue.vuex.constants';
import { createFacility } from './api';

export default {
name: 'CreateNewFacilityModal',
mixins: [commonCoreStrings, commonSyncElements],
data() {
return {
facilityName: '',
preset: Presets.NONFORMAL,
Presets,
};
},
computed: {
facilityNameInvalid() {
return !this.facilityName || this.facilityName.trim() === '';
},
},
methods: {
handleSubmit() {
if (this.facilityNameInvalid) {
return this.$refs.facilityNameTextBox.focus();
}
const payload = {
name: this.facilityName,
preset: this.preset,
};
createFacility(payload)
.then(() => {
this.$emit('success');
this.showSnackbarNotification('newLearningFacilityCreated');
})
.catch(error => {
this.$store.dispatch('handleApiError', error);
});
},
},
$trs: {
facilityNameFieldLabel: {
message: 'Learning facility name',
context: 'The field where the admin adds the name of their facility.',
},
learningEnvironmentHeader: {
message: 'What kind of environment is your facility?',
context: 'Title for facility environment.',
},
formalLabel: {
message: 'Formal',
context: 'Label for the radio button option in the facility setup.',
},
formalDescription: {
message: 'Schools and other formal learning contexts.',
context: "Option description text for 'Formal' facility types.",
},
nonFormalLabel: {
message: 'Non-formal',
context: 'Label for the radio button option in the facility setup',
},
nonFormalDescription: {
message:
'Libraries, orphanages, youth centers, computer labs, and other non-formal learning contexts.',

context: "Option description text for 'Non-formal' facility types.",
},
createFacilityLabel: {
message: 'Create a new learning facility',
context: 'Title for create facility modal',
},
createFacilityButtonLabel: {
message: 'create facility',
context: 'Label for create facility submit button.',
},
},
};

</script>


<style lang="scss" scoped></style>
12 changes: 12 additions & 0 deletions kolibri/plugins/device/assets/src/views/FacilitiesPage/api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import client from 'kolibri.client';
import urls from 'kolibri.urls';

const url = urls['kolibri:core:facility-create-facility']();

export function createFacility(payload) {
return client({
url,
method: 'POST',
data: payload,
});
}
58 changes: 54 additions & 4 deletions kolibri/plugins/device/assets/src/views/FacilitiesPage/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,18 @@
@click="showSyncAllModal = true"
/>
<KButton
:text="getCommonSyncString('importFacilityAction')"
hasDropdown
:text="$tr('createFacilityLabel')"
primary
style="margin-top: 16px; margin-bottom: -16px;"
@click="showImportModal = true"
/>
>
<template #menu>
<KDropdownMenu
:options="options"
@select="handleSelect"
/>
</template>
</KButton>
</KButtonGroup>
</template>
</HeaderWithOptions>
Expand Down Expand Up @@ -144,6 +151,11 @@
@success="handleStartImportSuccess"
@cancel="showImportModal = false"
/>
<CreateNewFacilityModal
v-if="showCreateFacilityModal"
@success="handleCreateFacilitySuccess"
@cancel="showCreateFacilityModal = false"
/>

<!-- NOTE similar code for KDP Registration in SyncInterface -->
<template v-if="Boolean(facilityForRegister)">
Expand Down Expand Up @@ -195,13 +207,14 @@
import { TaskStatuses, TaskTypes } from 'kolibri.utils.syncTaskUtils';
import some from 'lodash/some';
import DeviceAppBarPage from '../DeviceAppBarPage';
import { PageNames } from '../../constants';
import { PageNames, ImportFacility, CreateNewFacility } from '../../constants';
import { deviceString } from '../commonDeviceStrings';
import TasksBar from '../ManageContentPage/TasksBar';
import HeaderWithOptions from '../HeaderWithOptions';
import RemoveFacilityModal from './RemoveFacilityModal';
import SyncAllFacilitiesModal from './SyncAllFacilitiesModal';
import ImportFacilityModalGroup from './ImportFacilityModalGroup';
import CreateNewFacilityModal from './CreateNewFacilityModal';
import facilityTaskQueue from './facilityTasksQueue';

const Options = Object.freeze({
Expand All @@ -221,6 +234,7 @@
DeviceAppBarPage,
ConfirmationRegisterModal,
CoreTable,
CreateNewFacilityModal,
HeaderWithOptions,
FacilityNameAndSyncStatus,
ImportFacilityModalGroup,
Expand All @@ -235,6 +249,7 @@
return {
showSyncAllModal: false,
showImportModal: false,
showCreateFacilityModal: false,
facilities: [],
facilityForSync: null,
facilityForRemoval: null,
Expand All @@ -245,6 +260,18 @@
};
},
computed: {
options() {
return [
{
label: this.$tr('importFacilityLabel'),
value: ImportFacility,
},
{
label: this.$tr('createNewFacilityLabel'),
value: CreateNewFacility,
},
];
},
Copy link
Member

@akolson akolson Sep 12, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the label values should be translated strings.

pageTitle() {
return deviceString('deviceManagementTitle');
},
Expand Down Expand Up @@ -344,6 +371,10 @@
});
this.showImportModal = false;
},
handleCreateFacilitySuccess() {
this.showCreateFacilityModal = false;
this.fetchFacilites();
},
manageSync(facilityId) {
return {
name: PageNames.MANAGE_SYNC_SCHEDULE,
Expand Down Expand Up @@ -389,6 +420,13 @@
this.$emit('failure');
});
},
handleSelect(option) {
if (option.value == ImportFacility) {
this.showImportModal = true;
} else {
this.showCreateFacilityModal = true;
}
},
},
$trs: {
syncAllAction: {
Expand All @@ -401,6 +439,18 @@
context:
"Notification that appears after a facility has been deleted. For example, \"Removed 'Zuk Village' from this device'.",
},
createFacilityLabel: {
message: 'ADD FACILITY',
context: 'Label for a button used to create new facility.',
},
importFacilityLabel: {
message: 'Import facility',
context: 'Label for the dropdown option of import facility',
},
createNewFacilityLabel: {
message: 'Create new facility',
context: 'Label for the dropdown option of create new facility',
},
},
};

Expand Down
8 changes: 0 additions & 8 deletions kolibri/plugins/setup_wizard/assets/src/constants.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
import permissionPresets from '../../../../core/auth/constants/facility_configuration_presets.json';

// aliasing 'informal' to 'personal' since it's how we talk about it
const Presets = Object.freeze({
PERSONAL: 'informal',
FORMAL: 'formal',
NONFORMAL: 'nonformal',
});

/**
* enum identifying whether the user has gone to the on my own flow or not
*/
Expand Down Expand Up @@ -43,7 +36,6 @@ export {
permissionPresets,
DeviceTypePresets,
FacilityTypePresets,
Presets,
UsePresets,
SoudQueue,
LodTypePresets,
Expand Down
Loading
Loading