Reviewer's Guide by Sourcery

This pull request implements a notification icon with a badge in the navigation bar to display non-critical warnings to the user. It also includes a test for the new notification badge feature.

Sequence diagram for the new warning notification flow

sequenceDiagram
    participant User
    participant App
    participant Navbar
    participant NotificationSystem

    App->>NotificationSystem: Initialize warnings state
    App->>Navbar: Render with notifications prop
    Note over Navbar: Display notification badge
    User->>Navbar: Click notification icon
    Navbar->>NotificationSystem: Open notification popover
    NotificationSystem-->>User: Display warning messages
    User->>NotificationSystem: View warnings
    User->>NotificationSystem: Close notification popover

Class diagram for updated Navbar component

classDiagram
    class Navbar {
        +boolean isLoggedIn
        +function onLogin()
        +string[] notifications
        -HTMLElement anchorEl
        -HTMLElement anchorMailEl
        -boolean openAccountMenu
        -boolean openMailMenu
        +handleClick(event)
        +handleClose()
        +handleMailClick(event)
        +handleMailClose()
    }
    note for Navbar "Added notifications support"

    class Badge {
        +number badgeContent
        +string color
    }

    class NotificationsIcon {
        +string color
    }

    Navbar --> Badge
    Navbar --> NotificationsIcon

File-Level Changes

Assessment against linked issues

✅ Deploy Preview for neurobagel-query ready!

Name	Link
🔨 Latest commit	aaef274
🔍 Latest deploy log	https://app.netlify.com/sites/neurobagel-query/deploys/67911d783462ab00080df256
😎 Deploy Preview	https://deploy-preview-432--neurobagel-query.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Hi @Tusharjamdade,

Congratulations on your first contribution to Neurobagel! 🎉 The changes look great overall. However, there are a few areas that need adjustments:

• Currently, the implementation doesn't support deleting notifications, either individually or all at once.
• The way warnings are passed to the Navbar component is too specific, limiting reusability.
• Some of the e2e and component tests are failing due to these changes.

To address these, we can:

• Implement delete functionality for each notification and a "clear all notifications" feature for the popover.

• Refactor the logic for passing notifications to the Navbar component to make it more generic. This will allow us to pass different types of notifications (info, errors) to the Navbar.

  • For that the type of notification so that the appropriate title and color can be set in the ListItemText.
  • Once the logic is updated, we can also display info notifications in the popover.

• Update the tests to reflect these changes.

I understand this might be overwhelming for your first PR and contribution. Please don't hesitate to reach out with any questions, or let us know if you would prefer one of the maintainers to take over this PR.

Thank you for the review, @rmanaem.

I appreciate the detailed feedback and suggestions.
I will work on the details shared and will reach out if I need any help!!

Hi @Tusharjamdade,

Congratulations on your first contribution to Neurobagel! 🎉 The changes look great overall. However, there are a few areas that need adjustments:

• Currently, the implementation doesn't support deleting notifications, either individually or all at once.
• The way warnings are passed to the Navbar component is too specific, limiting reusability.
• Some of the e2e and component tests are failing due to these changes.

To address these, we can:

• Implement delete functionality for each notification and a "clear all notifications" feature for the popover.

• Refactor the logic for passing notifications to the Navbar component to make it more generic. This will allow us to pass different types of notifications (info, errors) to the Navbar.

  • For that the type of notification so that the appropriate title and color can be set in the ListItemText.
  • Once the logic is updated, we can also display info notifications in the popover.

• Update the tests to reflect these changes.

I understand this might be overwhelming for your first PR and contribution. Please don't hesitate to reach out with any questions, or let us know if you would prefer one of the maintainers to take over this PR.

Hi @rmanaem

export type Notification = {
  id: number;
  type: 'info' | 'warning';
  message: string;
};

const [notifications, setNotifications] = useState<Notification[]>([]);

<Navbar
  isLoggedIn={isAuthenticated}
  onLogin={() => setOpenAuthDialog(true)}
  notifications={notifications}
  setNotifications={setNotifications}
/>

// Navbar component
function Navbar({
  isLoggedIn,
  onLogin,
  notifications,
  setNotifications
}: {
  isLoggedIn: boolean;
  onLogin: () => void;
  notifications: Notification[];
  setNotifications: React.Dispatch<React.SetStateAction<Notification[]>>;
}) 

This is an overview of the implementation I have used. I have stored all notifications based on their types (warning, info) and messages, assigning each one a unique identifier (which will be used to delete the notification when the delete icon is clicked).

I passed the notifications and setNotifications to update the notifications (when the user clicks the delete button or the clear all button). Is this approach acceptable, or do I need to work on something else?

Hi @Tusharjamdade,
That's a good approach. Could you push your changes so I can take a look at them in context?

Hi @Tusharjamdade, That's a good approach. Could you push your changes so I can take a look at them in context?

Hi @rmanaem,

I am encountering a strange error

It checks whether the enqueueSnackbar contains, for example, NoAssessmentNode, but this value was never passed to the enqueueSnackbar function as a warning in the App.tsx file or any other file. As a result, the test fails because it is looking for NoAssessmentNode, which does not exist. I modified the enqueueSnackbar function and added all the warnings to the notification, but since the value NoAssessmentNode was never passed to the notification, the test still fails.
Am I missing something?

it('Shows warning for node that failed Assessment tool option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoAssessmentNode');
  });
  it('Shows warning for node that failed Diagnosis option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoDiagnosisNode');
  });
  it('Shows warning for node that failed Pipeline option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoPipelineNode');
  });
  it('Shows warning for node that failed Pipeline version option request', () => {
    cy.get('[data-cy="close-auth-dialog-button"]').click();
    cy.get('[data-cy="Pipeline name-categorical-field"]').type('fmri{downarrow}{enter}');
    cy.wait('@getPipelineVersionsOptions');
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoPipelineVersionNode');
  });

Hi @Tusharjamdade, That's a good approach. Could you push your changes so I can take a look at them in context?

Hi @rmanaem,

I am encountering a strange error

It checks whether the enqueueSnackbar contains, for example, NoAssessmentNode, but this value was never passed to the enqueueSnackbar function as a warning in the App.tsx file or any other file. As a result, the test fails because it is looking for NoAssessmentNode, which does not exist. I modified the enqueueSnackbar function and added all the warnings to the notification, but since the value NoAssessmentNode was never passed to the notification, the test still fails. Am I missing something?

it('Shows warning for node that failed Assessment tool option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoAssessmentNode');
  });
  it('Shows warning for node that failed Diagnosis option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoDiagnosisNode');
  });
  it('Shows warning for node that failed Pipeline option request', () => {
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoPipelineNode');
  });
  it('Shows warning for node that failed Pipeline version option request', () => {
    cy.get('[data-cy="close-auth-dialog-button"]').click();
    cy.get('[data-cy="Pipeline name-categorical-field"]').type('fmri{downarrow}{enter}');
    cy.wait('@getPipelineVersionsOptions');
    cy.get('.notistack-SnackbarContainer')
      .find('.notistack-MuiContent-warning')
      .should('contain', 'NoPipelineVersionNode');
  });

I have resolved this issue and updated the changes. Could you please review them?

Hi @rmanaem,
I have implemented the suggested changes. Could you please review them?