Skip to content

[CRUD] Add basic tests#4709

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

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

Conversation

@apedroferreira
Copy link
Collaborator

@apedroferreira apedroferreira commented Feb 27, 2025

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
Copy link

mui-bot commented Feb 27, 2025

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) => {
Copy link
Member

@Janpot Janpot Mar 11, 2025

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
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
15 checks passed
@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