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

Refactor admin app to prepare v5 migration #425

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

fzaninotto
Copy link
Contributor

The admin code was hard to dive in for a regular react-admin developer. I applied the conventions we use in react-admin to make it more maintaineable.

  • Smaller files
  • one component per file
  • group files by domain
  • barrel file for resources

This will facilitate the react-admin v5 migration.

Q A
Branch? main
Tickets no ticket
License MIT
Doc PR

@fzaninotto
Copy link
Contributor Author

I don't understand the linter failure. It tries to validate a file that was moved elsewhere...

@vincentchalamon
Copy link
Contributor

Hi @fzaninotto,

Thanks for this refactor, it needed it! As a backend developer, I tried my best to have an admin almost correct, so don't hesitate if you find some other fixes to do in the pwa.

I fixed the CI on main, can you rebase from it to fix it in your PR please?

@vincentchalamon vincentchalamon merged commit 22da274 into api-platform:main Jul 12, 2024
5 of 7 checks passed
@vincentchalamon
Copy link
Contributor

Thanks @fzaninotto

@fzaninotto fzaninotto deleted the admin-refactor branch July 12, 2024 07:56
@fzaninotto
Copy link
Contributor Author

Thanks for merging! However, I see that some e2e tests aren't passing. I don't know how to launch them locally, so I'd appreciate a hand to fix these tests.

@vincentchalamon
Copy link
Contributor

Thanks for merging! However, I see that some e2e tests aren't passing. I don't know how to launch them locally, so I'd appreciate a hand to fix these tests.

E2E tests may suffer false-errors due to GitHub Runners performances.

To run the E2E tests locally, I first set a .env file at the root path of the project with the following contents:

AUTH_SECRET=3xVEsEb/fD0k5EyVbERzQcIWvgNnko5DVPpHbZdyXlk=
CADDY_MERCURE_JWT_SECRET=!ChangeThisMercureHubJWTSecretKey!
PHP_DOCKER_IMAGE=api-platform-demo/php
PWA_DOCKER_IMAGE=api-platform-demo/pwa
KEYCLOAK_DOCKER_IMAGE=api-platform-demo/keycloak
APP_SECRET=427dff72d1aebf47de03ec08f012d369f4bde2d2
POSTGRES_PASSWORD=!ChangeMe!
KEYCLOAK_POSTGRES_PASSWORD=!ChangeMe!
KEYCLOAK_ADMIN_PASSWORD=!ChangeMe!
COMPOSE_FILE=compose.yaml:compose.prod.yaml:compose.e2e.yaml

Note: most of those environment variables are only required in compose.prod.yaml, but E2E tests run on iso-prod environment (cf. COMPOSE_FILE environment variable).

Then I build the images iso-prod:

docker compose build

And start the project:

docker compose up --wait

Must load the fixtures before any call:

docker compose exec php bin/console doctrine:fixtures:load --no-interaction

Finally, the E2E tests can be ran:

cd pwa
pnpm install
pnpm exec playwright test --ui

@vincentchalamon
Copy link
Contributor

vincentchalamon commented Jul 12, 2024

Currently, the rating filter on admin/reviews list has disappeared: https://github.com/api-platform/demo/actions/runs/9904379649/job/27362430448

And my too little knownledge on TS and React-Admin doesn't help me to fix it 😄

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.

2 participants