FORMS-1707: decouple forms from current user, optimize performance (b…

…cgov#1579) Get Current User always returns form data even when not used. Make the fetch of user form data explicit by adding new API to fetch the current user forms. Signed-off-by: Jason Sherman <[email protected]>
bhuvan-aot · Jan 20, 2025 · e6204e2 · e6204e2
1 parent 2dcf1ae
commit e6204e2
Show file tree

Hide file tree

Showing 12 changed files with 246 additions and 36 deletions.
diff --git a/app/frontend/src/components/admin/Developer.vue b/app/frontend/src/components/admin/Developer.vue
@@ -25,7 +25,8 @@ onBeforeMount(async () => {
 async function getUser() {
   try {
     const user = await rbacService.getCurrentUser();
-    apiRes.value = user.data;
+    const forms = await rbacService.getCurrentUserForms();
+    apiRes.value = { ...user.data, forms: forms.data };
   } catch (error) {
     notificationStore.addNotification({
       text: t('trans.developer.notificationMsg'),

diff --git a/app/frontend/src/services/rbacService.js b/app/frontend/src/services/rbacService.js
@@ -15,8 +15,16 @@ export default {
    * Get the current user details from the rbac endpoint
    * @returns {Promise} An axios response
    */
-  getCurrentUser(params = {}) {
-    return appAxios().get(`${ApiRoutes.RBAC}/current`, { params });
+  getCurrentUser() {
+    return appAxios().get(`${ApiRoutes.RBAC}/current`, {});
+  },
+  /**
+   * @function getCurrentUserForms
+   * Get the current user's forms from the rbac endpoint
+   * @returns {Promise} An axios response
+   */
+  getCurrentUserForms(params = {}) {
+    return appAxios().get(`${ApiRoutes.RBAC}/current/forms`, { params });
   },
 
   /**

diff --git a/app/frontend/src/store/form.js b/app/frontend/src/store/form.js
@@ -156,10 +156,10 @@ export const useFormStore = defineStore('form', {
     async getFormsForCurrentUser() {
       try {
         // Get the forms based on the user's permissions
-        const response = await rbacService.getCurrentUser();
+        const response = await rbacService.getCurrentUserForms();
         const data = response.data;
         // Build up the list of forms for the table
-        const forms = data.forms.map((f) => ({
+        const forms = data.map((f) => ({
           currentVersionId: f.formVersionId,
           id: f.formId,
           idps: f.idps,
@@ -183,10 +183,12 @@ export const useFormStore = defineStore('form', {
       try {
         this.permissions = [];
         // Get the forms based on the user's permissions
-        const response = await rbacService.getCurrentUser({ formId: formId });
+        const response = await rbacService.getCurrentUserForms({
+          formId: formId,
+        });
         const data = response.data;
-        if (data.forms[0]) {
-          this.permissions = data.forms[0].permissions;
+        if (data[0]) {
+          this.permissions = data[0].permissions;
         } else {
           throw new Error('No form found');
         }
@@ -205,10 +207,12 @@ export const useFormStore = defineStore('form', {
       try {
         this.roles = [];
         // Get the forms based on the user's permissions
-        const response = await rbacService.getCurrentUser({ formId: formId });
+        const response = await rbacService.getCurrentUserForms({
+          formId: formId,
+        });
         const data = response.data;
-        if (data.forms[0]) {
-          this.roles = data.forms[0].roles;
+        if (data[0]) {
+          this.roles = data[0].roles;
         } else {
           throw new Error('No form found');
         }

diff --git a/app/frontend/tests/unit/components/admin/Developer.spec.js b/app/frontend/tests/unit/components/admin/Developer.spec.js
@@ -11,6 +11,7 @@ import Developer from '~/components/admin/Developer.vue';
 describe('Developer.vue', () => {
   const mockConsoleError = vi.spyOn(console, 'error');
   const getCurrentUserSpy = vi.spyOn(rbacService, 'getCurrentUser');
+  const getCurrentUserFormsSpy = vi.spyOn(rbacService, 'getCurrentUserForms');
 
   const pinia = createTestingPinia();
   setActivePinia(pinia);
@@ -28,7 +29,12 @@ describe('Developer.vue', () => {
   });
 
   it('renders without error', async () => {
-    const data = {};
+    const userData = { id: 'a' };
+    const formsData = [];
+    const resData = {
+      ...userData,
+      forms: formsData,
+    };
     authStore.keycloak = {
       tokenParsed: {
         email: '[email protected]',
@@ -38,7 +44,8 @@ describe('Developer.vue', () => {
       token: 'token',
       fullName: 'fullName',
     };
-    getCurrentUserSpy.mockImplementation(() => ({ data: data }));
+    getCurrentUserSpy.mockImplementation(() => ({ data: userData }));
+    getCurrentUserFormsSpy.mockImplementation(() => ({ data: formsData }));
     const wrapper = mount(Developer, {
       global: {
         props: {
@@ -56,7 +63,7 @@ describe('Developer.vue', () => {
 
     expect(wrapper.text()).toMatch('Developer Resources');
     expect(getCurrentUserSpy).toHaveBeenCalledTimes(1);
-    expect(wrapper.vm.apiRes).toEqual(data);
+    expect(wrapper.vm.apiRes).toEqual(resData);
   });
 
   it('renders with error', async () => {

diff --git a/app/frontend/tests/unit/services/rbacService.spec.js b/app/frontend/tests/unit/services/rbacService.spec.js
@@ -25,7 +25,19 @@ describe('RBAC Service', () => {
     it('calls rbac/current endpoint', async () => {
       mockAxios.onGet(endpoint).reply(200);
 
-      const result = await rbacService.getCurrentUser({ idp: 'idir' });
+      const result = await rbacService.getCurrentUser();
+      expect(result).toBeTruthy();
+      expect(mockAxios.history.get).toHaveLength(1);
+    });
+  });
+
+  describe('rbac/current/forms', () => {
+    const endpoint = `${ApiRoutes.RBAC}/current/forms`;
+
+    it('calls rbac/current endpoint', async () => {
+      mockAxios.onGet(endpoint).reply(200);
+
+      const result = await rbacService.getCurrentUserForms({ idp: 'idir' });
       expect(result).toBeTruthy();
       expect(mockAxios.history.get).toHaveLength(1);
       expect(Object.keys(mockAxios.history.get[0].params)).toEqual(['idp']);

diff --git a/app/src/docs/v1.api-spec.yaml b/app/src/docs/v1.api-spec.yaml
@@ -2351,8 +2351,32 @@ paths:
                 $ref: '#/components/schemas/Error'
   /rbac/current:
     get:
-      summary: Get forms/roles/permissions for current user
+      summary: Get current user details
       operationId: getCurrentUser
+      tags:
+        - RBAC
+      responses:
+        '200':
+          description: OK
+          content:
+            application/json:
+              schema:
+                allOf:
+                  - $ref: '#/components/schemas/CurrentUser'
+        '401':
+          $ref: '#/components/responses/Unauthorized'
+        '403':
+          $ref: '#/components/responses/Forbidden'
+        default:
+          description: Unexpected error
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
+  /rbac/current/forms:
+    get:
+      summary: Get forms/roles/permissions for current user
+      operationId: getCurrentUserForms
       tags:
         - RBAC
       parameters:
@@ -2385,7 +2409,6 @@ paths:
             application/json:
               schema:
                 allOf:
-                  - $ref: '#/components/schemas/CurrentUser'
                   - type: array
                     items:
                       $ref: '#/components/schemas/UserForm'

diff --git a/app/src/forms/rbac/controller.js b/app/src/forms/rbac/controller.js
@@ -44,9 +44,15 @@ module.exports = {
   },
   getCurrentUser: async (req, res, next) => {
     try {
-      const response = await service.getCurrentUser(req.currentUser, req.query);
-      // don't want this going out, only need deleted forms on current user in middleware.
-      delete response.deletedForms;
+      const response = await service.getCurrentUser(req.currentUser);
+      res.status(200).json(response);
+    } catch (error) {
+      next(error);
+    }
+  },
+  getCurrentUserForms: async (req, res, next) => {
+    try {
+      const response = await service.getCurrentUserForms(req.currentUser, req.query);
       res.status(200).json(response);
     } catch (error) {
       next(error);

diff --git a/app/src/forms/rbac/routes.js b/app/src/forms/rbac/routes.js
@@ -12,6 +12,10 @@ routes.get('/current', jwtService.protect(), async (req, res, next) => {
   await controller.getCurrentUser(req, res, next);
 });
 
+routes.get('/current/forms', jwtService.protect(), async (req, res, next) => {
+  await controller.getCurrentUserForms(req, res, next);
+});
+
 routes.get('/current/submissions', jwtService.protect(), async (req, res, next) => {
   await controller.getCurrentUserSubmissions(req, res, next);
 });

diff --git a/app/src/forms/rbac/service.js b/app/src/forms/rbac/service.js
@@ -66,25 +66,32 @@ const service = {
     return User.query().findById(id).throwIfNotFound();
   },
 
-  getCurrentUser: async (currentUser, params) => {
+  getCurrentUser: async (currentUser) => {
     const user = Object.assign({}, currentUser);
-    const accessLevels = [];
-    if (user.public) {
-      accessLevels.push('public');
-    } else {
-      if (params.public) accessLevels.push('public');
-      if (params.idp) accessLevels.push('idp');
-      if (params.team) accessLevels.push('team');
-    }
+    return user;
+  },
 
-    const forms = await authService.getUserForms(user, {
-      ...params,
-      active: true,
-    });
-    const filteredForms = authService.filterForms(user, forms, accessLevels);
-    user.forms = filteredForms;
+  getCurrentUserForms: async (currentUser, params = {}) => {
+    if (!currentUser) return [];
+    try {
+      const accessLevels = [];
+      if (currentUser.public) {
+        accessLevels.push('public');
+      } else {
+        if (params.public) accessLevels.push('public');
+        if (params.idp) accessLevels.push('idp');
+        if (params.team) accessLevels.push('team');
+      }
 
-    return user;
+      const forms = await authService.getUserForms(currentUser, {
+        ...params,
+        active: true,
+      });
+      const filteredForms = authService.filterForms(currentUser, forms, accessLevels);
+      return filteredForms;
+    } catch {
+      return [];
+    }
   },
 
   getCurrentUserSubmissions: async (currentUser, params) => {

diff --git a/app/tests/unit/forms/rbac/controller.spec.js b/app/tests/unit/forms/rbac/controller.spec.js
@@ -36,3 +36,19 @@ describe('setSubmissionUserPermissions', () => {
     expect(service.modifySubmissionUser).toBeCalledWith(req.query.formSubmissionId, req.query.userId, req.body, req.currentUser);
   });
 });
+
+describe('getCurrentUserForms', () => {
+  const req = {
+    query: { formId: '1' },
+    body: { permissions: [] },
+    currentUser: {},
+    headers: { referer: 'a' },
+  };
+  it('should call the service with the query params', async () => {
+    service.getCurrentUserForms = jest.fn().mockReturnValue([]);
+    await controller.getCurrentUserForms(req, {}, jest.fn());
+
+    expect(service.getCurrentUserForms).toBeCalledTimes(1);
+    expect(service.getCurrentUserForms).toBeCalledWith(req.currentUser, req.query);
+  });
+});
diff --git a/app/tests/unit/forms/rbac/routes.spec.js b/app/tests/unit/forms/rbac/routes.spec.js
@@ -83,6 +83,27 @@ describe(`${basePath}/current`, () => {
   });
 });
 
+describe(`${basePath}/current/forms`, () => {
+  const path = `${basePath}/current/forms`;
+
+  it('should have correct middleware for GET', async () => {
+    controller.getCurrentUserForms = jest.fn((_req, res) => {
+      res.sendStatus(200);
+    });
+
+    await appRequest.get(path);
+
+    expect(controller.getCurrentUserForms).toBeCalledTimes(1);
+    expect(hasFormPermissionsMock).toBeCalledTimes(0);
+    expect(hasFormRolesMock).toBeCalledTimes(0);
+    expect(hasSubmissionPermissionsMock).toBeCalledTimes(0);
+    expect(mockJwtServiceProtect).toBeCalledTimes(1);
+    expect(userAccess.currentUser).toBeCalledTimes(1);
+    expect(userAccess.hasRoleDeletePermissions).toBeCalledTimes(0);
+    expect(userAccess.hasRoleModifyPermissions).toBeCalledTimes(0);
+  });
+});
+
 describe(`${basePath}/current/submissions`, () => {
   const path = `${basePath}/current/submissions`;