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

enhance: introduce private and public preview feature flags #4473

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

sjschlapbach
Copy link
Member

@sjschlapbach sjschlapbach commented Jan 23, 2025

Summary by CodeRabbit

Release Notes

  • Feature Changes

    • Introduced separate public and private preview flags for users.
    • Updated navigation and feature visibility based on new preview flags.
    • Refined terminology from "question" to "element" across the application.
  • Localization Updates

    • Updated German and English language files to reflect new terminology.
    • Replaced "question" references with more generic "element" terminology.
  • Backend Modifications

    • Updated GraphQL schema to support public and private preview checks.
    • Modified user model to include distinct preview flags.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 23, 2025
Copy link

coderabbitai bot commented Jan 23, 2025

📝 Walkthrough

Walkthrough

This pull request introduces a comprehensive refactoring of feature preview functionality across the application. The changes primarily involve replacing a single featurePreview flag with two distinct flags: publicPreview and privatePreview. This modification impacts multiple components, GraphQL schema, localization files, and database schema, ensuring a more granular approach to user feature access and visibility.

Changes

File Path Change Summary
apps/frontend-manage/src/components/common/Header.tsx Updated navigation logic to use user?.privatePreview for rendering menu items.
apps/frontend-manage/src/components/courses/* Replaced user?.featurePreview with user?.publicPreview in multiple components.
apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx Updated GraphQL query import and usage from CheckFeaturePreviewAvailableDocument to CheckPublicPreviewAvailableDocument.
packages/graphql/src/schema/query.ts Replaced checkFeaturePreviewAvailable with checkPublicPreviewAvailable and checkPrivatePreviewAvailable.
packages/graphql/src/schema/user.ts Removed featurePreview, added publicPreview and privatePreview.
packages/prisma/src/prisma/schema/user.prisma Updated User model to replace featurePreview with publicPreview and privatePreview.
packages/i18n/messages/* Updated localization keys from "question" to "element" terminology.
packages/prisma/src/data/helpers.ts Updated prepareUser function to accept publicPreview and privatePreview parameters instead of featurePreview.
packages/prisma/src/data/seedUsers.ts Adjusted user seeding to remove featurePreview and include publicPreview and privatePreview.
packages/prisma/src/prisma/migrations/* Updated SQL migration to rename featurePreview to publicPreview and add privatePreview.
cypress/cypress.config.ts Added updateLecturerPermissions method to update user permissions based on preview flags.
cypress/cypress/e2e/B-feature-access-workflow.cy.ts New tests for validating feature access based on user permissions.
cypress/cypress/fixtures/* Added new fixture for feature access and removed outdated fixture.

Possibly related PRs

Suggested reviewers

  • rschlaefli

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d356ce and cade875.

📒 Files selected for processing (1)
  • packages/prisma/src/data/seedUsers.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/prisma/src/data/seedUsers.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: cypress-run
  • GitHub Check: check
  • GitHub Check: Analyze (javascript)
  • GitHub Check: build
  • GitHub Check: SonarCloud

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dosubot dosubot bot added the enhancement label Jan 23, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (6)
packages/prisma/src/data/seedUsers.ts (1)

60-69: Consider documenting the preview flag access model.

The seed data establishes different levels of preview access:

  • Standard users: both previews
  • Individual pro: public preview only
  • Institutional pro: private preview only
  • Free users: no previews

Consider adding a comment explaining this access model for future maintainers.

apps/frontend-manage/src/components/questions/manipulation/useElementTypeOptions.ts (1)

76-86: Consider error handling for the preview check.

While the implementation is correct, consider handling potential query errors to ensure graceful degradation.

-  if (data?.checkPrivatePreviewAvailable) {
+  if (!error && data?.checkPrivatePreviewAvailable) {
     baseElements.push({
       value: ElementType.Selection,
       label: t(`shared.${ElementType.Selection}.typeLabel`),
       data: {
         cy: `select-question-type-${t(
           `shared.${ElementType.Selection}.typeLabel`
         )}`,
       },
     })
   }
apps/frontend-manage/src/components/courses/MicroLearningElement.tsx (1)

Line range hint 366-378: LGTM: Analytics link visibility is correctly gated behind publicPreview flag.

The conditional rendering of the ActivityAnalyticsLink component using user?.publicPreview is consistent across both published and ended states.

Consider extracting the duplicated analytics link condition into a separate function to reduce code duplication:

+ const analyticsLinkItem = (courseId: string, activityId: string) =>
+   user?.publicPreview
+     ? {
+         label: (
+           <ActivityAnalyticsLink
+             courseId={courseId}
+             activityId={activityId}
+           />
+         ),
+         onClick: () => null,
+       }
+     : []

  // Then use it in both places:
- user?.publicPreview
-   ? {
-       label: (
-         <ActivityAnalyticsLink
-           courseId={courseId}
-           activityId={microLearning.id}
-         />
-       ),
-       onClick: () => null,
-     }
-   : [],
+ analyticsLinkItem(courseId, microLearning.id),

Also applies to: 447-459

packages/graphql/src/services/accounts.ts (1)

948-958: Consider reducing code duplication with checkPublicPreviewAvailable.

The implementation is nearly identical to checkPublicPreviewAvailable. Consider extracting the common logic into a shared helper function.

-export async function checkPublicPreviewAvailable(ctx: Context) {
+async function checkPreviewAvailable(ctx: Context, previewType: 'publicPreview' | 'privatePreview') {
   // check if user is logged in
   if (!ctx.user?.sub || ctx.user?.role === UserRole.PARTICIPANT) {
     return false
   }
 
   const user = await ctx.prisma.user.findUnique({
     where: { id: ctx.user.sub },
   })
 
-  return user?.publicPreview ?? false
+  return user?.[previewType] ?? false
 }
 
+export async function checkPublicPreviewAvailable(ctx: Context) {
+  return checkPreviewAvailable(ctx, 'publicPreview')
+}
+
 export async function checkPrivatePreviewAvailable(ctx: Context) {
-  // check if user is logged in
-  if (!ctx.user?.sub || ctx.user?.role === UserRole.PARTICIPANT) {
-    return false
-  }
-
-  const user = await ctx.prisma.user.findUnique({
-    where: { id: ctx.user.sub },
-  })
-
-  return user?.privatePreview ?? false
+  return checkPreviewAvailable(ctx, 'privatePreview')
 }
packages/prisma/src/prisma/schema/user.prisma (1)

97-98: Add default value to privatePreview for consistency.

While the migration sets a default value for privatePreview, the schema doesn't reflect this. Consider adding @default(false) to privatePreview to maintain consistency with publicPreview and align with the migration.

  publicPreview  Boolean @default(false)
- privatePreview Boolean @default(false)
+ privatePreview Boolean @default(false)
apps/frontend-manage/src/components/questions/tags/TagList.tsx (1)

90-102: Consider memoizing elementTypeFilters.

Since elementTypeFilters depends only on data?.checkPrivatePreviewAvailable, consider using useMemo to prevent unnecessary recalculations on re-renders.

- const elementTypeFilters: Record<ElementType, IconDefinition[] | undefined> =
-   {
-     CONTENT: [faCommentRegular, faCommentSolid],
-     FLASHCARD: [faListRegular, faListSolid],
-     SC: [faQuestionRegular, faQuestionSolid],
-     MC: [faQuestionRegular, faQuestionSolid],
-     KPRIM: [faQuestionRegular, faQuestionSolid],
-     FREE_TEXT: [faQuestionRegular, faQuestionSolid],
-     NUMERICAL: [faQuestionRegular, faQuestionSolid],
-     SELECTION: data?.checkPrivatePreviewAvailable
-       ? [faSquareCheckRegular, faSquareCheckSolid]
-       : undefined,
-   }
+ const elementTypeFilters = useMemo(
+   () => ({
+     CONTENT: [faCommentRegular, faCommentSolid],
+     FLASHCARD: [faListRegular, faListSolid],
+     SC: [faQuestionRegular, faQuestionSolid],
+     MC: [faQuestionRegular, faQuestionSolid],
+     KPRIM: [faQuestionRegular, faQuestionSolid],
+     FREE_TEXT: [faQuestionRegular, faQuestionSolid],
+     NUMERICAL: [faQuestionRegular, faQuestionSolid],
+     SELECTION: data?.checkPrivatePreviewAvailable
+       ? [faSquareCheckRegular, faSquareCheckSolid]
+       : undefined,
+   }),
+   [data?.checkPrivatePreviewAvailable]
+ ) as Record<ElementType, IconDefinition[] | undefined>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ddef42b and 86d688f.

⛔ Files ignored due to path filters (9)
  • packages/graphql/src/graphql/ops/QCheckFeaturePreviewAvailable.graphql is excluded by !**/**/graphql/ops/**
  • packages/graphql/src/graphql/ops/QCheckPrivatePreviewAvailable.graphql is excluded by !**/**/graphql/ops/**
  • packages/graphql/src/graphql/ops/QCheckPublicPreviewAvailable.graphql is excluded by !**/**/graphql/ops/**
  • packages/graphql/src/graphql/ops/QUserProfile.graphql is excluded by !**/**/graphql/ops/**
  • packages/graphql/src/ops.schema.json is excluded by !**/**/ops.schema.json
  • packages/graphql/src/ops.ts is excluded by !**/**/ops.ts
  • packages/graphql/src/public/client.json is excluded by !**/**/public/**
  • packages/graphql/src/public/schema.graphql is excluded by !**/**/public/**
  • packages/graphql/src/public/server.json is excluded by !**/**/public/**
📒 Files selected for processing (18)
  • apps/frontend-manage/src/components/common/Header.tsx (1 hunks)
  • apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx (1 hunks)
  • apps/frontend-manage/src/components/courses/MicroLearningElement.tsx (2 hunks)
  • apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (1 hunks)
  • apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx (3 hunks)
  • apps/frontend-manage/src/components/questions/manipulation/ElementFormErrors.tsx (1 hunks)
  • apps/frontend-manage/src/components/questions/manipulation/ElementInformationFields.tsx (2 hunks)
  • apps/frontend-manage/src/components/questions/manipulation/useElementTypeOptions.ts (2 hunks)
  • apps/frontend-manage/src/components/questions/tags/TagList.tsx (4 hunks)
  • packages/graphql/src/schema/query.ts (1 hunks)
  • packages/graphql/src/schema/user.ts (1 hunks)
  • packages/graphql/src/services/accounts.ts (2 hunks)
  • packages/i18n/messages/de.ts (1 hunks)
  • packages/i18n/messages/en.ts (1 hunks)
  • packages/prisma/src/data/helpers.ts (1 hunks)
  • packages/prisma/src/data/seedUsers.ts (4 hunks)
  • packages/prisma/src/prisma/migrations/20250123091154_multiple_preview_flags/migration.sql (1 hunks)
  • packages/prisma/src/prisma/schema/user.prisma (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: SonarCloud
  • GitHub Check: format
  • GitHub Check: cypress-run
  • GitHub Check: check
  • GitHub Check: Analyze (javascript)
  • GitHub Check: build
  • GitHub Check: test
  • GitHub Check: build
🔇 Additional comments (21)
packages/prisma/src/data/seedUsers.ts (2)

21-22: LGTM! Clear assignment of both preview flags for the standard user.

The standard user has been granted access to both public and private previews, which aligns with the PR's objective of introducing granular feature preview controls.


34-34: LGTM! Appropriate preview flag distribution for pro users.

The preview flags are logically distributed:

  • Individual pro user gets publicPreview
  • Institutional pro user gets privatePreview

This separation aligns well with the PR's objective of providing granular feature access control.

Also applies to: 46-46

apps/frontend-manage/src/components/questions/manipulation/useElementTypeOptions.ts (1)

10-12: LGTM! Appropriate cache strategy for preview check.

The use of cache-first fetch policy is optimal for this scenario as preview status rarely changes during a session.

packages/graphql/src/schema/user.ts (1)

39-40: LGTM! Clean implementation of preview flags in the User type.

The separation into publicPreview and privatePreview boolean fields provides clear and type-safe access control.

Verify the impact on existing queries.

Let's check for queries that might still be using the old featurePreview field.

✅ Verification successful

No existing queries found using the old featurePreview field

The only references to featurePreview are UI-related translations and display components. The schema changes can be safely implemented without breaking existing queries.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to featurePreview in GraphQL queries
rg -t graphql "featurePreview" -A 5

Length of output: 67


Script:

#!/bin/bash
# Search for featurePreview across all relevant file types
echo "=== Searching in TypeScript/JavaScript files ==="
rg "featurePreview" --type-add 'gql:*.{graphql,gql}' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -t gql -C 3

echo -e "\n=== Searching for gql tagged template literals ==="
rg "gql.*featurePreview" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -C 3

Length of output: 1745

apps/frontend-manage/src/components/questions/manipulation/ElementInformationFields.tsx (1)

40-40: LGTM! Consistent terminology update.

The change from 'question' to 'element' in labels maintains consistency with the broader application terminology:

  • questionTypeelementType
  • questionTitleelementTitle

Verify terminology consistency across the application.

Let's check for any remaining instances of the old terminology.

Also applies to: 62-62

✅ Verification successful

Terminology update verified with minor exceptions

The change from 'question' to 'element' terminology is consistently applied in user-facing content:

  • Translation keys use 'elementTitle'
  • Form labels and error messages use 'elementTitle'
  • Test selectors (data-cy attributes) retain 'question-title' for test stability
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potentially missed terminology updates
rg -t typescript -t javascript "questionType|questionTitle" -g "!*.test.*" -g "!*.spec.*"

Length of output: 126


Script:

#!/bin/bash
# Search for both old and new terminology in relevant files
echo "=== Searching for type terminology ==="
rg -g "*.{js,jsx,ts,tsx}" "questionType|elementType" -A 2 -B 2

echo -e "\n=== Searching for title terminology ==="
rg -g "*.{js,jsx,ts,tsx}" "questionTitle|elementTitle" -A 2 -B 2

Length of output: 120455


Script:

#!/bin/bash
# Search for title-related terminology with case variations
echo "=== Searching for title terminology ==="
rg -i -g "*.{tsx,jsx}" "(?:question|element)[\s_-]?title" -A 2 -B 2

echo -e "\n=== Searching in translation files ==="
rg -g "*.ts" "(?:question|element)[\s_-]?title" ./packages/i18n/

Length of output: 2666

apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx (2)

5-5: LGTM: Import statement updated correctly.

The import statement has been updated to use the new CheckPublicPreviewAvailableDocument query.


46-46: LGTM: Feature preview check updated consistently.

The query and its usage have been updated to use checkPublicPreviewAvailable instead of checkFeaturePreviewAvailable, maintaining the same conditional rendering logic for the analytics button.

Also applies to: 77-77

apps/frontend-manage/src/components/questions/manipulation/ElementFormErrors.tsx (1)

27-27: LGTM: Translation key updated for consistency.

The translation key has been updated from 'questionTitle' to 'elementTitle' as part of terminology standardization.

apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx (1)

88-88: LGTM: Feature preview check updated consistently.

The condition has been updated to use user?.publicPreview instead of user?.featurePreview, maintaining the same conditional rendering logic for the learning analytics button.

apps/frontend-manage/src/components/common/Header.tsx (2)

68-92: LGTM: Private preview menu items added correctly.

The resources and catalog menu items are now conditionally rendered based on user?.privatePreview. The implementation:

  • Maintains all original properties of the menu items
  • Preserves the notification badge for catalog sharing requests
  • Uses type assertion to ensure type safety

93-93: LGTM: Public preview analytics dropdown added correctly.

The analytics dropdown is now conditionally rendered based on user?.publicPreview. The implementation:

  • Maintains all original properties and functionality
  • Preserves the course-specific analytics options
  • Keeps the "older courses" link at the bottom

Also applies to: 94-144

apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (1)

Line range hint 389-401: LGTM: Analytics link visibility is correctly gated behind publicPreview flag.

The conditional rendering of the ActivityAnalyticsLink component using user?.publicPreview is consistent with the PR's objective of introducing granular preview flags.

packages/graphql/src/schema/query.ts (1)

724-736: LGTM: GraphQL queries for preview flags are well-structured.

The implementation correctly splits the feature preview check into two distinct queries with proper typing and consistent use of the builder pattern.

packages/prisma/src/data/helpers.ts (1)

30-31: LGTM: User preparation correctly includes new preview flags.

The prepareUser function signature has been updated to include both publicPreview and privatePreview as optional parameters, maintaining backward compatibility.

packages/graphql/src/services/accounts.ts (1)

Line range hint 935-946: LGTM! Well-implemented authorization check.

The function properly validates user authentication and role before checking the preview flag.

packages/i18n/messages/en.ts (1)

980-986: LGTM! Consistent terminology update.

The changes appropriately rename question-related keys to element-related keys, maintaining semantic meaning while aligning with the new terminology.

packages/prisma/src/prisma/migrations/20250123091154_multiple_preview_flags/migration.sql (1)

1-3: LGTM! Migration follows best practices.

The migration safely renames the existing column and adds a new one with appropriate constraints.

apps/frontend-manage/src/components/questions/tags/TagList.tsx (2)

87-89: LGTM! Appropriate cache policy for feature flag.

The query uses cache-first which is optimal for feature flags as they rarely change during a session.


167-186: LGTM! Proper null handling in map function.

The map function correctly handles undefined icons and uses early return pattern.

packages/i18n/messages/de.ts (2)

979-980: LGTM! Consistent terminology update from "Fragetyp" to "Elementtyp"

The change aligns with the broader refactoring to use more generic terminology that better represents all content types.


983-985: LGTM! Consistent terminology update from "Fragetitel" to "Elementtitel"

The change in both the key and tooltip text maintains consistency with the terminology update, replacing question-specific language with more generic element-based terms.

Copy link

cypress bot commented Jan 23, 2025

klicker-uzh    Run #4141

Run Properties:  status check passed Passed #4141  •  git commit 709bf9d18c ℹ️: Merge cade8753a1601700936c340f9d60daf6a3731640 into ddef42b6dc475b939d649f42a03e...
Project klicker-uzh
Branch Review new-preview-flags
Run status status check passed Passed #4141
Run duration 18m 00s
Commit git commit 709bf9d18c ℹ️: Merge cade8753a1601700936c340f9d60daf6a3731640 into ddef42b6dc475b939d649f42a03e...
Committer Julius Schlapbach
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 301
View all changes introduced in this branch ↗︎

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 23, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (7)
cypress/cypress.config.ts (2)

254-288: LGTM! Consider adding explicit error handling for user not found.

The implementation follows the established patterns and properly manages resources. However, consider adding explicit error handling for the case where the lecturer user is not found.

 try {
-  const user = await prisma.user.update({
-    where: {
-      shortname: 'lecturer',
-    },
-    data: {
-      publicPreview,
-      privatePreview,
-    },
-  })
+  const user = await prisma.user.update({
+    where: {
+      shortname: 'lecturer',
+    },
+    data: {
+      publicPreview,
+      privatePreview,
+    },
+  }).catch((error) => {
+    if (error.code === 'P2025') {
+      throw new Error('Lecturer user not found')
+    }
+    throw error
+  })

   return !!user

261-271: Consider refactoring prisma client initialization to reduce duplication.

The prisma client initialization code is duplicated across all database-related tasks. Consider extracting it into a reusable function.

function createPrismaClient() {
  if (!process.env.DATABASE_URL) {
    throw new Error('DATABASE_URL environment variable is not set')
  }

  return new PrismaClient({
    datasources: {
      db: {
        url: process.env.DATABASE_URL,
      },
    },
  })
}

Then use it in all tasks:

 async updateLecturerPermissions({
   publicPreview,
   privatePreview,
 }: {
   publicPreview: boolean
   privatePreview: boolean
 }) {
-  if (!process.env.DATABASE_URL) {
-    throw new Error('DATABASE_URL environment variable is not set')
-  }
-
-  const prisma = new PrismaClient({
-    datasources: {
-      db: {
-        url: process.env.DATABASE_URL,
-      },
-    },
-  })
+  const prisma = createPrismaClient()

Also applies to: 46-271

cypress/cypress/e2e/B-feature-access-workflow.cy.ts (3)

10-95: Consider refactoring the helper function for better maintainability.

The validateFeatureAvailability function could benefit from several improvements:

  1. Split into smaller, focused functions for each feature type (analytics, resources, question types)
  2. Add proper error handling for navigation failures
  3. Replace hardcoded waits with proper Cypress wait commands
  4. Extract duplicated selectors into constants

Example refactor for the analytics validation:

const SELECTORS = {
  analytics: '[data-cy="analytics"]',
  courses: '[data-cy="courses"]',
  learningAnalytics: '[data-cy="course-learning-analytics-link"]',
  // ... other selectors
};

function validateAnalyticsFeatures(data: any, publicPreview: boolean) {
  // Check analytics in menubar
  cy.get(SELECTORS.analytics)
    .should(publicPreview ? 'exist' : 'not.exist');

  // Check course analytics
  cy.get(SELECTORS.courses)
    .click()
    .get(`[data-cy="course-list-button-${data.seed.courseName}"]`)
    .click()
    .get(SELECTORS.learningAnalytics)
    .should(publicPreview ? 'exist' : 'not.exist');
}

136-182: Refactor permission update logic to reduce duplication.

The permission update logic is duplicated across multiple test cases. Consider extracting it into a custom command or helper function.

// In commands.ts
Cypress.Commands.add('updatePermissions', (publicPreview: boolean, privatePreview: boolean) => {
  return cy.task('updateLecturerPermissions', { publicPreview, privatePreview })
    .then((result: boolean) => {
      if (!result) {
        throw new Error('Permissions of user could not be updated.');
      }
      cy.loginLecturer();
      cy.reload();
    });
});

// In test file
it('Verify that only the public preview features are available if the corresponding flag is set', function () {
  cy.updatePermissions(true, false);
  validateFeatureAvailability({
    data: this.data,
    publicPreview: true,
    privatePreview: false,
  });
});

3-223: Add test cases for edge cases and error scenarios.

The test suite could be enhanced with additional test cases:

  1. Test behavior when permission update fails
  2. Test concurrent permission updates
  3. Test with invalid permission combinations
  4. Test recovery from navigation failures

Example additional test case:

it('Should handle permission update failures gracefully', function () {
  // Mock the task to simulate failure
  cy.task('updateLecturerPermissions', {
    publicPreview: true,
    privatePreview: true,
  }, { timeout: 1000 }).as('permissionUpdate');
  
  // Verify error handling
  cy.get('@permissionUpdate')
    .should('equal', false)
    .then(() => {
      // Verify UI shows appropriate error message
      cy.get('[data-cy="error-message"]')
        .should('contain', 'Failed to update permissions');
    });
});
apps/frontend-manage/src/components/questions/tags/TagList.tsx (2)

87-89: Consider caching strategy for preview check.

While using 'cache-first' is reasonable for feature flags, consider if 'cache-and-network' would be more appropriate to ensure users get the latest preview status without manual refresh.


Line range hint 1-187: Well-structured feature flag implementation.

The approach to feature flag implementation demonstrates good architectural decisions:

  1. Clear separation between flag checking (GraphQL) and UI rendering
  2. Graceful degradation when features are not available
  3. Type-safe implementation with proper TypeScript usage

Consider documenting the feature flag strategy for consistency across the codebase.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86d688f and 5d356ce.

📒 Files selected for processing (8)
  • apps/frontend-manage/src/components/common/Header.tsx (1 hunks)
  • apps/frontend-manage/src/components/questions/tags/TagItem.tsx (1 hunks)
  • apps/frontend-manage/src/components/questions/tags/TagList.tsx (4 hunks)
  • cypress/cypress.config.ts (1 hunks)
  • cypress/cypress/e2e/B-catalyst-workflow.cy.ts (0 hunks)
  • cypress/cypress/e2e/B-feature-access-workflow.cy.ts (1 hunks)
  • cypress/cypress/fixtures/B-catalyst.json (0 hunks)
  • cypress/cypress/fixtures/B-feature-access.json (1 hunks)
💤 Files with no reviewable changes (2)
  • cypress/cypress/fixtures/B-catalyst.json
  • cypress/cypress/e2e/B-catalyst-workflow.cy.ts
✅ Files skipped from review due to trivial changes (1)
  • cypress/cypress/fixtures/B-feature-access.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/frontend-manage/src/components/common/Header.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: SonarCloud
  • GitHub Check: build
  • GitHub Check: check
  • GitHub Check: test
  • GitHub Check: build
  • GitHub Check: cypress-run
🔇 Additional comments (4)
cypress/cypress/e2e/B-feature-access-workflow.cy.ts (1)

4-8: Verify the existence and content of the fixture file.

The test suite depends on B-feature-access.json fixture, but the file is not included in the PR. Please ensure this fixture file is added and contains the necessary test data.

✅ Verification successful

Fixture file verification successful

The B-feature-access.json fixture exists at the correct location with valid test data for feature access testing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the fixture file exists and validate its content
fd -t f "B-feature-access.json" cypress/fixtures/

Length of output: 154


Script:

#!/bin/bash
# Search for the fixture file and cypress config
echo "=== Searching for fixture file ==="
fd -t f "B-feature-access.json"

echo -e "\n=== Searching for Cypress config ==="
fd -t f "cypress.config"

Length of output: 297


Script:

#!/bin/bash
# Check the content of the fixture file
echo "=== Fixture file content ==="
cat cypress/cypress/fixtures/B-feature-access.json

Length of output: 253

apps/frontend-manage/src/components/questions/tags/TagItem.tsx (1)

10-10: LGTM! Well-structured test attribute implementation.

The addition of data-cy and data-test attributes enhances component testability while maintaining backward compatibility through optional properties.

Also applies to: 13-13, 21-22

apps/frontend-manage/src/components/questions/tags/TagList.tsx (2)

167-187: LGTM! Robust null handling with early return.

The implementation properly handles undefined icons with an early return, and the data-cy attribute follows a consistent naming convention.


90-102: Verify the impact of undefined SELECTION type.

The conditional rendering of SELECTION type based on private preview availability is well implemented. However, ensure that any code consuming elementTypeFilters handles the undefined case gracefully.

✅ Verification successful

Code safely handles undefined SELECTION type

The code properly handles the undefined case for SELECTION type. The map function explicitly checks for undefined icons before rendering TagItem components, returning null when icons are undefined. This implementation ensures there are no runtime errors when SELECTION type is unavailable.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct access to elementTypeFilters.SELECTION
rg -l "elementTypeFilters\W+SELECTION" --type ts --type tsx

# Search for usage of ElementType.SELECTION
rg -l "ElementType\.SELECTION" --type ts --type tsx

Length of output: 171


Script:

#!/bin/bash
# Search for elementTypeFilters usage
rg "elementTypeFilters\[.*SELECTION\]|elementTypeFilters\.SELECTION" --type typescript

# Search for ElementType.SELECTION usage
rg "ElementType\.SELECTION|type:\s*['\"]*SELECTION" --type typescript

# Search for any array destructuring or mapping of elementTypeFilters
rg "const.*=.*elementTypeFilters|\.map.*elementTypeFilters" --type typescript

# Use ast-grep to find object property access
ast-grep --pattern 'elementTypeFilters.$_'

Length of output: 392


Script:

#!/bin/bash
# Find and show the TagList.tsx file
fd TagList.tsx

# Show the content of TagList.tsx to analyze usage
fd TagList.tsx -x cat {}

# Search without file type restrictions
rg "elementTypeFilters" --no-ignore

# Search for SELECTION type usage
rg "SELECTION" --no-ignore

Length of output: 79763

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@sjschlapbach sjschlapbach merged commit 8171873 into v3-new-elements Jan 23, 2025
15 of 16 checks passed
@sjschlapbach sjschlapbach deleted the new-preview-flags branch January 23, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size:XL This PR changes 500-999 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

1 participant