Skip to content

feat: replace instruments table with the new dynamic material table #1793

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

martin-trajanovski
Copy link
Collaborator

@martin-trajanovski martin-trajanovski commented Mar 21, 2025

Description

Replaces the instruments table with the new dynamic material table component

Motivation

All the existing tables should be replaced by the new dynamic material table for better performance, flexibility and alignment.

Fixes:

Please provide a list of the fixes implemented in this PR

Changes:

Please provide a list of the changes implemented by this PR

  • Replaces the existing instruments table with the new dynamic material table for improved performance and flexibility.
  • Improves the handling of table settings, pagination, and sorting using the dynamic material table.

Tests included

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)

Documentation

  • swagger documentation updated [required]
  • official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

  • Does it require a specific version of the backend
  • which version of the backend is required:

Summary by Sourcery

Replaces the instruments table with the new dynamic material table component, and refactors the proposal dashboard to use the new dynamic material table component.

Enhancements:

  • Replaces the existing instruments table with the new dynamic material table for improved performance and flexibility.
  • Refactors the proposal dashboard to use the new dynamic material table component.
  • Improves the handling of table settings, pagination, and sorting using the dynamic material table.

Summary by Sourcery

Replace the existing instruments table with a new dynamic material table component, improving performance, flexibility, and user experience

New Features:

  • Implement a new dynamic material table for instruments dashboard
  • Add support for advanced table settings, pagination, and sorting

Enhancements:

  • Refactor instruments dashboard to use dynamic material table
  • Improve table interaction with query parameter-based navigation
  • Enhance table configuration and user settings persistence

Tests:

  • Update unit tests to reflect new table implementation
  • Modify test cases for pagination and sorting interactions

@martin-trajanovski martin-trajanovski marked this pull request as ready for review April 22, 2025 11:46
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 @martin-trajanovski - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider extracting the table configuration logic into a separate service or utility function to improve readability and maintainability.
  • The component now fetches data on every query parameter change; consider debouncing or throttling these requests to avoid excessive API calls.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 2 issues 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.

const { pageIndex: page, pageSize: limit } = event;
this.store.dispatch(changePageAction({ page, limit }));
ngOnInit(): void {
this.subscriptions.push(
Copy link

Choose a reason for hiding this comment

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

suggestion: Clarify the pending state management in multiple subscriptions.

Both the instruments data subscription and the queryParams subscription set 'this.pending'. It might be prudent to separate concerns or better coordinate the pending state updates to avoid potential race conditions or inconsistent UI states.

Suggested implementation:

  ) {}

  // New pending flags for separate subscriptions
  private instrumentPending: boolean = true;
  private queryPending: boolean = true;
  // Helper method to update the overall pending state
  private updatePendingState(): void {
    this.pending = this.instrumentPending || this.queryPending;
  }

  ngOnInit(): void {
    this.subscriptions.push(
      this.instrumentsWithCountAndTableSettings$.subscribe(
        ({ instruments, count, tablesSettings }) => {
          this.tablesSettings = tablesSettings;
          this.dataSource.next(instruments);
          this.instrumentPending = false;
          this.updatePendingState();

You will need to update any other subscription (for example, the queryParams subscription) that currently sets this.pending:
• Replace its direct assignment (e.g. this.pending = false) with setting its own flag (e.g. this.queryPending = false) followed by a call to updatePendingState().
This will separate the pending state management for different subscriptions, avoiding potential race conditions or inconsistent UI states.

customMetadata: this.jsonHeadPipe.transform(instrument.customMetadata),
})),
})),
export class InstrumentsDashboardComponent implements OnInit, OnDestroy {
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 component to use RxJS's takeUntil for subscription management and move table configuration logic to a dedicated service or utility to reduce its responsibilities and improve focus.

Consider refactoring to reduce component responsibilities:

1. **Simplify Subscription Management:**  
   Replace the manual subscriptions array with RxJS’s `takeUntil`. For example, add a private subject and pipe your observables with `takeUntil(this.unsubscribe$)`:

   ```typescript
   private unsubscribe$ = new Subject<void>();

   ngOnInit(): void {
     this.instrumentsWithCountAndTableSettings$
       .pipe(takeUntil(this.unsubscribe$))
       .subscribe(({ instruments, count, tablesSettings }) => {
         // ... existing logic
       });

     this.route.queryParams
       .pipe(takeUntil(this.unsubscribe$))
       .subscribe(queryParams => {
         // ... existing query param handling
       });
   }

   ngOnDestroy(): void {
     this.unsubscribe$.next();
     this.unsubscribe$.complete();
   }
  1. Extract Table Configuration Logic:
    Move helper methods like getTableSort, getTablePaginationConfig, and initTable into a dedicated service or utility. For instance:

    // table-config.service.ts
    @Injectable({ providedIn: 'root' })
    export class TableConfigService {
      getTableSort(route: ActivatedRoute): ITableSetting["tableSort"] {
        const { queryParams } = route.snapshot;
        return queryParams.sortDirection && queryParams.sortColumn
          ? { sortColumn: queryParams.sortColumn, sortDirection: queryParams.sortDirection }
          : null;
      }
    
      getPaginationConfig(route: ActivatedRoute, dataCount: number, defaultPageSize: number, defaultOptions: number[]): TablePagination {
        const { queryParams } = route.snapshot;
        return {
          pageSizeOptions: defaultOptions,
          pageIndex: queryParams.pageIndex,
          pageSize: queryParams.pageSize || defaultPageSize,
          length: dataCount,
        };
      }
    }

    Then, inject and use it in your component:

    constructor(
      private store: Store,
      private router: Router,
      private route: ActivatedRoute,
      private tableConfigService: TableConfigService
    ) {}
    
    ngOnInit(): void {
      // ...
      const tableSort = this.tableConfigService.getTableSort(this.route);
      const paginationConfig = this.tableConfigService.getPaginationConfig(this.route, count, this.defaultPageSize, this.defaultPageSizeOptions);
      // ...
    }

These steps isolate concerns and make the component code more focused.

@@ -195,6 +195,29 @@ Cypress.Commands.add("createProposal", (proposal) => {
});
});
});

Cypress.Commands.add("createInstrument", (instrument) => {
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 flattening the nested .then calls in createInstrument and removeInstruments by returning promises to improve readability and maintainability of the code..

Actionable Comment:

The additional nested .then() calls can be flattened by returning each promise to chain them instead of nesting callbacks. For example, refactor createInstrument as follows:

Cypress.Commands.add("createInstrument", (instrument) => {
  return cy.getCookie("user")
    .then((userCookie) => {
      const user = JSON.parse(decodeURIComponent(userCookie.value));
      return user; // pass the user along
    })
    .then((user) =>
      cy.getToken().then((token) => ({ user, token }))
    )
    .then(({ user, token }) => {
      cy.log("Instrument: " + JSON.stringify(instrument, null, 2));
      cy.log("User: " + JSON.stringify(user, null, 2));
      return cy.request({
        method: "POST",
        url: lbBaseUrl + "/Instruments",
        headers: {
          Authorization: token,
          Accept: "application/json",
          "Content-Type": "application/json",
        },
        body: instrument,
      });
    });
});

Similarly, in removeInstruments, avoid nesting by returning promises:

Cypress.Commands.add("removeInstruments", () => {
  cy.login(Cypress.env("username"), Cypress.env("password"));
  return cy.getToken()
    .then((token) =>
      cy.request({
        method: "GET",
        url: lbBaseUrl + "/instruments",
        headers: {
          Authorization: token,
          Accept: "application/json",
          "Content-Type": "application/json",
        },
      })
    )
    .its("body")
    .then((instruments) => {
      cy.login(
        Cypress.env("secondaryUsername"),
        Cypress.env("secondaryPassword")
      );
      return cy.getToken()
        .then((token) => {
          // Use Cypress._.each if you want to maintain chainability
          return Cypress._.each(instruments, (instrument) => {
            cy.request({
              method: "DELETE",
              url: lbBaseUrl + "/instruments/" + encodeURIComponent(instrument.pid),
              headers: {
                Authorization: token,
                Accept: "application/json",
                "Content-Type": "application/json",
              },
            });
          });
        });
    });
});

Steps to Reduce Complexity:

  1. Return Promises: Ensure each .then() returns a promise so that you can chain further operations rather than nesting.
  2. Flatten Callbacks: Combine sequential asynchronous operations into a single chain.
  3. Optional: Consider using helper functions for repeated logic (like setting headers).

These changes keep the functionality intact while making the code easier to read and maintain.

Comment on lines +196 to +198
const { name, display, index, width } = column;

return { name, display, index, width };
Copy link

Choose a reason for hiding this comment

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

suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)

Suggested change
const { name, display, index, width } = column;
return { name, display, index, width };
return column;


ExplanationSomething that we often see in people's code is assigning to a result variable
and then immediately returning it.

Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.

Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.

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