Skip to content

[CRUD] Add basic tests - #4709

Merged
apedroferreira merged 9 commits into
mui:masterfrom
apedroferreira:crud-tests
Mar 13, 2025
Merged

[CRUD] Add basic tests#4709
apedroferreira merged 9 commits into
mui:masterfrom
apedroferreira:crud-tests

Conversation

@apedroferreira

@apedroferreira apedroferreira commented Feb 27, 2025

Copy link
Copy Markdown
Collaborator

Add tests for CRUD component.

Covers:

  • list view
  • detail view
  • creating items
  • editing items
  • deleting items from list view
  • deleting items from detail view.

@apedroferreira apedroferreira self-assigned this Feb 27, 2025
@mui-bot

mui-bot commented Feb 27, 2025

Copy link
Copy Markdown

Netlify deploy preview

https://deploy-preview-4709--mui-toolpad-docs.netlify.app/

Generated by 🚫 dangerJS against 8fa7c9c

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 5, 2025
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 7, 2025
@apedroferreira
apedroferreira marked this pull request as ready for review March 7, 2025 17:10
@apedroferreira
apedroferreira requested a review from a team March 7, 2025 17:10
],
getMany: async ({ paginationModel, filterModel, sortModel }) => {
getMany: ({ paginationModel, filterModel, sortModel }) => {
return new Promise((resolve) => {

@Janpot Janpot Mar 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using an all encompassing promise constructor with callback code inside is an anti-pattern. This should really be written as

  getMany: async ({ paginationModel, filterModel, sortModel }) => {
    await new Promise((resolve) => {
      setTimeout(resolve, 750);
    });

    let processedNotes = [...notesStore];

    // Apply filters (demo only)
    if (filterModel?.items?.length) {
      filterModel.items.forEach(({ field, value, operator }) => {
        if (!field || value == null) {
          return;
        }

        processedNotes = processedNotes.filter((note) => {
          const noteValue = note[field];

          switch (operator) {
            case 'contains':
              return String(noteValue)
                .toLowerCase()
                .includes(String(value).toLowerCase());
            case 'equals':
              return noteValue === value;
            case 'startsWith':
              return String(noteValue)
                .toLowerCase()
                .startsWith(String(value).toLowerCase());
            case 'endsWith':
              return String(noteValue)
                .toLowerCase()
                .endsWith(String(value).toLowerCase());
            case '>':
              return noteValue > value;
            case '<':
              return noteValue < value;
            default:
              return true;
          }
        });
      });
    }

    // Apply sorting
    if (sortModel?.length) {
      processedNotes.sort((a, b) => {
        for (const { field, sort } of sortModel) {
          if (a[field] < b[field]) {
            return sort === 'asc' ? -1 : 1;
          }
          if (a[field] > b[field]) {
            return sort === 'asc' ? 1 : -1;
          }
        }
        return 0;
      });
    }

    // Apply pagination
    const start = paginationModel.page * paginationModel.pageSize;
    const end = start + paginationModel.pageSize;
    const paginatedNotes = processedNotes.slice(start, end);

    return {
      items: paginatedNotes,
      itemCount: processedNotes.length,
    };
  },

everywhere. In most cases, code inside of a promise constructure should be strictly reduced to the callback calling function in practice, then be composed as promises.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok I can change that so that not everything is wrapped in the Promise, using await just for the timeout sounds good.

@prakhargupta1 prakhargupta1 moved this to In progress in Toolpad public roadmap Mar 12, 2025
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 12, 2025
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 12, 2025
@apedroferreira
apedroferreira merged commit d84a682 into mui:master Mar 13, 2025
@github-project-automation github-project-automation Bot moved this from In progress to Completed in Toolpad public roadmap Mar 13, 2025
@apedroferreira
apedroferreira deleted the crud-tests branch March 13, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants