Refactor facilityconfig facility actions into composables #13014

ozer550 · 2025-01-20T07:48:01Z

Summary

WIP
…

References

…

Reviewer guidance

…

github-actions · 2025-01-20T08:20:41Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a0.dev0_git.403.gba1efed8.pex
Windows Installer (EXE)	kolibri-0.18.0a0.dev0+git.403.gba1efed8-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a0.dev0+git.403.gba1efed8-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a0.dev0+git.403.gba1efed8.dmg
Android Package (APK)	kolibri-0.18.0a0.dev0+git.403.gba1efed8-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.0a0.dev0+git.403.gba1efed8.zip
TAR file	kolibri-0.18.0a0.dev0+git.403.gba1efed8.tar.gz
WHL file	kolibri-0.18.0a0.dev0+git.403.gba1efed8-py2.py3-none-any.whl

ozer550 · 2025-01-21T07:10:25Z

packages/kolibri-common/composables/useFacilities.js

+    getFacilityConfig,
+    setFacilityConfig,
+    setFacilities,
+    selectedFacility,


should we also migrate selectedFacility everywhere?

How is it usually handled? Is it in the core vuex state, or has it been independently implemented in each of the plugins?

So the existing getFacilityConfig actions was getting the value of selectedFacility from here. Hence I migrated it in our composable. there are multiple places where it is actually being implemented independently.

OK, I think we can do the selectedFacility migration as a follow up issue, if you'd like to have a go at writing that up? Feel free to run a draft by me!

ozer550 · 2025-01-21T07:11:13Z

packages/kolibri-common/composables/useFacilities.js

+const state = reactive({
+  facilityConfig: {},
+  facilities: [],
+  facilityId: Lockr.get('facilityId') || null,


Have not removed the existing facilitiyId state just added this here to keep a local copy of facilityId.

Hrm, but we're not actually setting this anywhere either. Let's remove this, it's not in the original, so not sure that we need it, and if we're not setting it, it's not doing anything anyway.

Its value is being used here. Any suggestions on how should I work around this?

rtibbles

This looks mostly how I would expect! A couple of comments where we could do a little additional cleanup.

I tend to distrust reactive, in general individual bits of state using ref are more predictable reactivity wise, hence my preference for that.

rtibbles · 2025-01-21T15:39:34Z

packages/kolibri-tools/test/fixtures/TestUserPermissions.js

@@ -17,6 +17,8 @@ import FacilityUserResource from 'kolibri-common/apiResources/FacilityUserResour
 import samePageCheckGenerator from 'kolibri-common/utils/samePageCheckGenerator';
 import { createTranslator } from 'kolibri/utils/i18n';
 import useUser from 'kolibri/composables/useUser';
+import useFacilities from 'kolibri-common/composables/useFacilities';
+import { getFacilities } from '../../../../kolibri/core/assets/src/state/modules/core/actions';


This function has been deleted from here, not sure why we've added this import?

rtibbles · 2025-01-21T15:40:15Z

packages/kolibri-common/composables/useFacilities.js

+    getFacilityConfig,
+    setFacilityConfig,
+    setFacilities,
+    selectedFacility,


How is it usually handled? Is it in the core vuex state, or has it been independently implemented in each of the plugins?

rtibbles · 2025-01-21T15:40:47Z

packages/kolibri-tools/test/fixtures/TestUserPermissions.js

@@ -63,6 +65,8 @@ function fetchUserPermissions(userId) {
 * @returns Promise<void>
 */
 export function showUserPermissionsPage(store, userId) {
+  const { getfacilities } = useFacilities();


This should be getFacilities - maybe that's why you had to add the import above?

rtibbles · 2025-01-21T15:46:13Z

packages/kolibri-common/composables/useFacilities.js

+const state = reactive({
+  facilityConfig: {},
+  facilities: [],
+  facilityId: Lockr.get('facilityId') || null,


Hrm, but we're not actually setting this anywhere either. Let's remove this, it's not in the original, so not sure that we need it, and if we're not setting it, it's not doing anything anyway.

rtibbles · 2025-01-21T15:47:57Z

packages/kolibri-common/composables/useFacilities.js

+import FacilityDatasetResource from 'kolibri-common/apiResources/FacilityDatasetResource';
+import Lockr from 'lockr';
+
+const state = reactive({


I would prefer that we not exactly map the vuex state here.

I think it would be cleaner to define each bit of state as a separate ref.

So:

const facilityConfig = ref({}); const facilities = ref([]); const facilityId = ref(null);

Would then need to update the state access and update accordingly.

If you want to keep them encapsulated and not externally editable, you could prefix their names with _ and still return computed props instead.

rtibbles · 2025-01-21T15:49:57Z

kolibri/plugins/user_auth/assets/src/modules/signUp/handlers.js

 import { ComponentMap } from '../../constants';

 export function showSignUpPage(store, fromRoute) {
+  const { setFacilities } = useFacilities();
+
  // Don't do anything if going between Sign Up steps
  if (fromRoute.name === ComponentMap.SIGN_UP) {
    return Promise.resolve();
  }
  return FacilityResource.fetchCollection()


Hrm, do you know if there's a reason we don't just use getFacilities here?

The existing functionality was using set facilities, I just replaced it. Are you suggesting that instead of calling the fetchCollection we directly call getFacilities here?

ohh yes! it does the same thing will replace!

rtibbles

Code wise this looks how I expect, and I have no concerns. This will be ready for QA once it has been undrafted and has a complete PR description! Please make sure to describe the specific workflows that are impacted by these changes to allow for complete testing.

rtibbles · 2025-01-23T17:46:28Z

packages/kolibri-common/composables/useFacilities.js

+    getFacilityConfig,
+    setFacilityConfig,
+    setFacilities,
+    selectedFacility,


OK, I think we can do the selectedFacility migration as a follow up issue, if you'd like to have a go at writing that up? Feel free to run a draft by me!

ozer550 added 5 commits January 16, 2025 14:37

migrate getFacilities action

d7a1129

migrate getFacilityConfig action

edae304

migrate remianing getters and mutations

c4415b8

make composable default export

c98efb8

fix tests and migrate facilityConfig getter

c258070

migrate facilities state

ab006c2

ozer550 requested a review from rtibbles January 21, 2025 06:56

ozer550 commented Jan 21, 2025

View reviewed changes

rtibbles reviewed Jan 21, 2025

View reviewed changes

register PR review

075ae88

rtibbles self-assigned this Jan 23, 2025

rtibbles reviewed Jan 23, 2025

View reviewed changes

ozer550 added 2 commits January 24, 2025 00:30

fix missed facilityConfig import in useCreate page

65ffec1

migrate userisMultiFacilityAdmin getter

4be93c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor facilityconfig facility actions into composables #13014

Refactor facilityconfig facility actions into composables #13014

ozer550 commented Jan 20, 2025

github-actions bot commented Jan 20, 2025 •

edited

Loading

ozer550 Jan 21, 2025

rtibbles Jan 21, 2025

ozer550 Jan 22, 2025

rtibbles Jan 23, 2025

ozer550 Jan 21, 2025

rtibbles Jan 21, 2025

ozer550 Jan 22, 2025

rtibbles left a comment

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

rtibbles Jan 21, 2025

ozer550 Jan 22, 2025

ozer550 Jan 22, 2025

rtibbles left a comment

rtibbles Jan 23, 2025

Refactor facilityconfig facility actions into composables #13014

Are you sure you want to change the base?

Refactor facilityconfig facility actions into composables #13014

Conversation

ozer550 commented Jan 20, 2025

Summary

References

Reviewer guidance

github-actions bot commented Jan 20, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2025 •

edited

Loading