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

fix: added sample safety reviewers permission to submit review on sample safetyTest pr review #1

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Junjiequan
Copy link
Owner

@Junjiequan Junjiequan commented Oct 1, 2024

Description

Currently, Safety Review Manager cannot be able to submit review to the submitted samples due to permission error

Added sample safety reviewers permission to submit review on sample safety

Motivation and Context

Updated Roles and Permissions to the Safety Review Manager.

How Has This Been Tested

e2e

Fixes

Changes

Depends on

Tests included/Docs Updated?

  • [*] I have added tests to cover my changes.
  • All relevant doc has been updated

Summary by Sourcery

Fix permission issues for sample safety reviewers by introducing a new mutation submitSampleReview to handle sample reviews. Refactor frontend components to simplify preview logic and remove unnecessary props. Add tests to ensure the new functionality works as expected.

New Features:

  • Introduce a new mutation submitSampleReview to allow sample safety reviewers to submit reviews with safety status and comments.

Bug Fixes:

  • Fix permission issues preventing sample safety reviewers from submitting reviews on samples.

Enhancements:

  • Refactor the PreviewTemplateModal component to simplify the rendering logic by removing the templateGroupId parameter.
  • Remove the previewMode prop from various container components to streamline the component interfaces.

Tests:

  • Add an end-to-end test to verify that sample safety reviewers can evaluate and submit reviews on samples.

Copy link

sourcery-ai bot commented Oct 1, 2024

Reviewer's Guide by Sourcery

This pull request implements changes to allow Sample Safety Reviewers to submit reviews on sample safety. The main changes include updating the backend to handle sample safety reviews, modifying the frontend to support this new functionality, and adjusting the authorization logic. Additionally, some refactoring has been done to remove unused code and improve the overall structure of the application.

Sequence Diagram

sequenceDiagram
    participant User as Sample Safety Reviewer
    participant Frontend
    participant Backend
    participant DB as Database

    User->>Frontend: Submit sample safety review
    Frontend->>Backend: submitSampleReview mutation
    Backend->>Backend: Authorize user
    Backend->>DB: Update sample safety status and comment
    DB-->>Backend: Confirm update
    Backend-->>Frontend: Return updated sample
    Frontend-->>User: Display success message
Loading

File-Level Changes

Change Details Files
Implemented sample safety review submission functionality
  • Added new mutation 'submitSampleReview' in the backend
  • Created a new GraphQL mutation for submitting sample reviews
  • Updated SampleDataSource to include a 'submitReview' method
  • Modified the frontend to use the new submitSampleReview mutation
  • Updated authorization logic to allow Sample Safety Reviewers to edit samples
apps/backend/src/mutations/ReviewMutations.ts
apps/backend/src/datasources/postgres/SampleDataSource.ts
apps/backend/src/resolvers/mutations/SubmitSampleReviewMutation.ts
apps/frontend/src/graphql/review/submitSampleReview.graphql
apps/frontend/src/components/sample/SampleSafetyPage.tsx
apps/backend/src/auth/SampleAuthorization.ts
Removed unused code and refactored components
  • Removed unused preview functionality from various components
  • Deleted unused methods for creating stubs
  • Simplified PreviewTemplateModal component
  • Removed unnecessary parameters from component props
apps/frontend/src/components/template/PreviewTemplateModal.tsx
apps/frontend/src/components/proposalEsi/ProposalEsiContainer.tsx
apps/frontend/src/components/questionary/questionaryComponents/SampleDeclaration/SampleDeclarationContainer.tsx
apps/frontend/src/components/questionary/questionaryComponents/GenericTemplate/GenericTemplateContainer.tsx
apps/frontend/src/components/shipments/ShipmentContainer.tsx
Updated tests to reflect new sample safety review functionality
  • Modified existing tests to use the new submitSampleReview mutation
  • Added new test cases for Sample Safety Reviewer permissions
  • Updated mock data sources to support new functionality
apps/backend/src/mutations/SampleMutations.spec.ts
apps/e2e/cypress/e2e/samples.cy.ts
apps/backend/src/datasources/mockups/SampleDataSource.ts
Adjusted data models and GraphQL schemas
  • Removed safety-related fields from updateSample mutation
  • Updated Sample data model to reflect new review submission process
  • Modified GraphQL schemas to support new review submission functionality
apps/backend/src/resolvers/mutations/UpdateSampleMutation.ts
apps/frontend/src/graphql/samples/updateSample.graphql
apps/backend/src/datasources/SampleDataSource.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Junjiequan - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Review instructions: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

</IconButton>
</Tooltip>
);
// NOTE: For now preview works on proposal templates only. Another task is added to make it work for all of the templates.
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider generalizing preview functionality

While it's good that you've added a note about the limitation, consider creating a more general solution that can handle previews for all template types, not just proposals. This would make the code more flexible and easier to maintain in the long run.

const canPreview = (templateGroupId: TemplateGroupId) => {
  // TODO: Implement preview functionality for all template types
  return templateGroupId === TemplateGroupId.PROPOSAL;
};

if (canPreview(state.groupId)) {
  topControlBarElements.push(

@@ -625,6 +625,80 @@ context('Samples tests', () => {
cy.contains('HIGH_RISK'); // test if status has changed
});

it('Sample Safety Reviewer should be able to evaluate sample', function () {
Copy link

Choose a reason for hiding this comment

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

issue (complexity): Consider refactoring the test cases to reduce code duplication and improve maintainability.

The new test case for the Sample Safety Reviewer introduces unnecessary complexity through significant code duplication. To reduce complexity while maintaining specific role testing, consider the following improvements:

  1. Extract common setup steps into shared functions:
function createSampleAndSubmitProposal() {
  cy.createSample({
    proposalPk: createdProposalPk,
    templateId: createdSampleTemplateId,
    questionId: createdSampleQuestionId,
    title: sampleTitle,
  });
  cy.submitProposal({ proposalPk: createdProposalPk });
}

function performSafetyReview(status, comment) {
  cy.get('[data-cy="safety-status"]').click();
  cy.contains(status).click();
  if (comment) {
    cy.get('[data-cy="safety-comment"]').type(comment);
  }
  cy.get('[data-cy="submit"]').click();
}
  1. Refactor the tests to use these shared functions:
it('User should be able to review sample safety', function () {
  // ... existing setup code ...
  createSampleAndSubmitProposal();
  cy.login(user);
  // ... navigation to sample ...
  performSafetyReview('High risk');
  cy.contains('HIGH_RISK');
});

it('Sample Safety Reviewer should be able to evaluate sample', function () {
  if (!featureFlags.getEnabledFeatures().get(FeatureId.SAMPLE_SAFETY)) {
    this.skip();
  }
  createSampleAndSubmitProposal();
  const sampleSafetyReviewer = initialDBData.users.user1;
  cy.updateUserRoles({
    id: sampleSafetyReviewer.id,
    roles: [initialDBData.roles.sampleSafetyReviewer],
  });
  cy.login(sampleSafetyReviewer);
  // ... navigation to sample ...
  performSafetyReview('Low risk', safetyComment);
  // ... verification steps ...
  performSafetyReview('High risk');
  cy.contains('HIGH_RISK');
});

These changes will significantly reduce code duplication, improve maintainability, and make it easier to add future tests for different user roles or scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant