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 1 commit
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
16 changes: 16 additions & 0 deletions kolibri/core/auth/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from django.contrib.auth.models import AnonymousUser
from django.core.exceptions import ObjectDoesNotExist
from django.core.exceptions import PermissionDenied
from django.db import transaction
from django.db.models import Func
from django.db.models import OuterRef
from django.db.models import Q
Expand Down Expand Up @@ -60,6 +61,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 Down Expand Up @@ -621,6 +623,20 @@ def annotate_queryset(self, queryset):
)
)

@decorators.action(methods=["post"], detail=False)
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!

if serializer.is_valid():
with transaction.atomic():
facility_dataset = FacilityDataset.objects.create(
preset=serializer.validated_data.get("preset")
)
Facility.objects.create(
name=serializer.validated_data.get("name"), dataset=facility_dataset
)
return Response()
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)


class PublicFacilityViewSet(viewsets.ReadOnlyModelViewSet):
queryset = Facility.objects.all()
Expand Down
9 changes: 9 additions & 0 deletions kolibri/core/auth/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from rest_framework import serializers
from rest_framework.validators import UniqueTogetherValidator

from .constants import facility_presets
from .errors import IncompatibleDeviceSettingError
from .errors import InvalidCollectionHierarchy
from .errors import InvalidMembershipError
Expand Down Expand Up @@ -137,6 +138,14 @@ 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.



class PublicFacilitySerializer(serializers.ModelSerializer):
learner_can_login_with_no_password = serializers.SerializerMethodField()
learner_can_sign_up = serializers.SerializerMethodField()
Expand Down
5 changes: 5 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,8 @@ 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 Presets = Object.freeze({
FORMAL: 'formal',
NONFORMAL: 'nonformal',
});
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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

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 have made some changes. Have a look and let me know if it needs any other changes.
Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<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 '../../constants';
import { createFacility } from './api';

export default {
name: 'CreateNewFacilityModal',
mixins: [commonCoreStrings, commonSyncElements],
data() {
return {
facilityName: '',
preset: 'nonformal',
Copy link
Member

Choose a reason for hiding this comment

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

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

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');
})
.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,
});
}
48 changes: 45 additions & 3 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 @@ -202,6 +214,7 @@
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: 'Import facility',
value: 'import_facility',
},
{
label: 'Create new facility',
value: 'create_new_facility',
},
];
},
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 == 'import_facility') {
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.

See my comments on value fields above

this.showImportModal = true;
} else {
this.showCreateFacilityModal = true;
}
},
},
$trs: {
syncAllAction: {
Expand All @@ -401,6 +439,10 @@
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.',
},
},
};

Expand Down
Loading