Skip to content

Shell imports#17

Merged
unflores merged 38 commits intomainfrom
shell-imports
Mar 10, 2025
Merged

Shell imports#17
unflores merged 38 commits intomainfrom
shell-imports

Conversation

@magrigsdev
Copy link
Collaborator

@magrigsdev magrigsdev commented Feb 19, 2025

add themes script
create themes.json
save on .json themes
no yet save on database
ParentId issue no yet fix


Deploy will require gd ext-zip extensions to be installed into php

@magrigsdev magrigsdev self-assigned this Feb 20, 2025
@magrigsdev magrigsdev requested a review from unflores February 20, 2025 02:01
@magrigsdev
Copy link
Collaborator Author

ericlouz c'est mon deuxième compte git. vraiment desolé poir le melange de compte

@unflores
Copy link
Owner

Mes retours sont pour le plupart structural et sur le nommage des fichiers et variables. Sinon, c'est cool que tu as quelquechose qui nous ammène bien vers la but. Bravo.

Voila mes retours:

Faut garder les parentId en integer et pas varchar. C'est mieux pour les jointures et aussi pour la structure de ne pas dependre d'une donnée de domaine.
Faut enlever les DS_Store
Ne garde pas les fichiers data dans le repo. Déjà, git n'est pas fait pour garder des larges fichiers bin. Normalement, tu rajoutes un dossier data dans gitignore et on garde les flux d'imports ailleurs.
Globalement, on ne va pas faire l'intermediaire du fichier json, et on va laisser les ids etre set par la bdd.
Fait attention de ne pas commettre des functions qui ne sont pas utilisées.
Fait attention de ne pas definir des functions en public si elles ne sont pas utilisées publiquement.
Attention à l'usage des mots comme data ou utils ou tout mot qui ne renseigne pas sur le but de la fonction ou variable.

Au niveau d'objets je pense qu'il y a plus de sens à avoir les objets suivants:
CommandIngestTheme
Script/IngestTheme => Imports\Themes\ExtractService
Script/SaveThemes => Imports\Themes\CreateService
CommandSaveTheme => redundant. à la poubelle

Au niveau de responsabilites:
CommandIngestTheme

  • agit comme le point d'entree
  • gere les arguments
  • inject le nom du fichier import

Imports\Themes\ExtractService

  • agit comme un service de transformation des données
  • retourne un tableau d'objets pret à etre persisté

Imports\Themes\CreateService

  • Fait la persistence
  • Gere une validation eventuelle

.gitignore Outdated
/public/assets/
/assets/vendor/
###< symfony/asset-mapper ###
.DS_Store
Copy link
Owner

Choose a reason for hiding this comment

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

Il faut effacer tout les fichiers .DS_Store

public function up(Schema $schema): void
{
// this up() migration is auto-generated, please modify it to your needs
$this->addSql('CREATE TABLE theme (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, parent_id INTEGER DEFAULT NULL, code VARCHAR(255) NOT NULL, is_section BOOLEAN NOT NULL, external_id VARCHAR(255) NOT NULL)');
Copy link
Owner

Choose a reason for hiding this comment

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

Je pense que ta bdd est en mauvaise etat comme la mienne ne necessite pas cette migration. En plus la fonctionne down suprimme la bdd 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

quand je vais merger je vais pas merger les migrations


protected function configure(): void
{
$this
Copy link
Owner

Choose a reason for hiding this comment

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

Cela n'a pas besoin d'exister.

@magrigsdev magrigsdev requested a review from unflores February 21, 2025 09:56
Comment on lines +45 to +66
if ($arg1) {
$io->note(sprintf('You passed an argument: %s', $arg1));
$excel_file = $this->projectDir.'/public/File/emissions_GES_structure.xlsx';

if (!file_exists($excel_file)) {
$io->error('file does not exist');

return Command::FAILURE;
}
try {
$themes = $ingestTheme->PrepareThemesForDatabase($ingestTheme->GetThemesFromExcelFile($excel_file));
$themes = json_decode($themes);

// $io->success('Résultat: '.$ingestTheme->checkThemesEmpty());
} catch (\Exception $e) {
$io->error('Erreur lors de la lecture du fichier : '.$e->getMessage());

return Command::FAILURE;
}

return Command::SUCCESS;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Si tu veux plus en discuter j'ai des dispos. Quand on a discuté de la sortie des données, on a dit que ce sera plutot en form de php dictionary et pas json. Changes l'output s'il te plait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oui je suis entrain de le modifier

Comment on lines +12 to +14
private $entityManager;
private $projectDir;

Copy link
Owner

Choose a reason for hiding this comment

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

ces changements n'ont pas besoin d'etre ici

private $projectDir;
private $entityManager;
private $themeRepository;

Copy link
Owner

Choose a reason for hiding this comment

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

le nom, signature de certaines methodes et le dossier de ce fichier doivent changer. Regarde mon feedback d'avant pour plus d'info.

return Command::FAILURE;
}
try {
$themes = $ingestTheme->PrepareThemesForDatabase($ingestTheme->GetThemesFromExcelFile($excel_file));
Copy link
Owner

Choose a reason for hiding this comment

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

La methode GetThemesFromExcelFile doit retourner qqch de ce format ci:

[
  'P.0' => [
    'code' => 'emission_ges',
    'parent_id' => null,
    'is_section' => true
  ],
  'P.0.1' => [
    'code' => 'emission_ges.some_child',
    'parent_id' => null,
    'is_section' => true
  ],
...
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dictionnaire de donnée.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

je vais finir les changement et tu vas les voir.

Copy link
Owner

@unflores unflores left a comment

Choose a reason for hiding this comment

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

il y a encore pas mal de changements à faire.

@magrigsdev magrigsdev requested a review from unflores February 25, 2025 10:17
@magrigsdev
Copy link
Collaborator Author

magrigsdev commented Feb 25, 2025

Could you take a moment to review this? I’ve finished everything, and it's all completed.

Comment on lines +37 to +38
$newSpreadsheet = new Spreadsheet();
$newSheet = $newSpreadsheet->getActiveSheet();
Copy link
Owner

Choose a reason for hiding this comment

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

Je pense que ceci doit etre effacé.

Comment on lines +205 to +216
private function getParentIdByparentExternalId(string $parentExternalId): ?int
{
$result = $this->entityManager->getRepository(Theme::class)
->createQueryBuilder('t')
->select('t.id')
->where('t.externalId = :externalId')
->setParameter('externalId', $parentExternalId)
->getQuery()
->getOneOrNullResult();

return $result['id'] ?? null;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Normalement ce genre de code dois vivre dans tes repositories, donc un theme repo.

Comment on lines +187 to +198
public function checkAllParentIdNotNull(): bool
{
$nullCount = $this->themeRepository->createQueryBuilder('t')
->select('COUNT(t.id)')
->where('t.id >= :startId')
->andWhere('t.parentId IS NOT NULL')
->setParameter('startId', 2)
->getQuery()
->getSingleScalarResult();

return 0 === $nullCount ? false : true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Je pense que ceci doit etre defini dans ton theme repository 🤔

{
$ExtractServices = new ExtractService($this->entityManager, $this->projectDir);
$saveThemes = $ExtractServices->SaveThemesOnDatabase();
$this->assertTrue($saveThemes, 'themes are not saved');
Copy link
Owner

Choose a reason for hiding this comment

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

les themes doivent etre sauvgardés normalement non?

@@ -1,27 +0,0 @@
<?php
Copy link
Owner

Choose a reason for hiding this comment

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

celle-la elle n'est pas à effacer.

@unflores unflores marked this pull request as ready for review March 10, 2025 09:37
@unflores unflores merged commit 95ecae0 into main Mar 10, 2025
1 check passed
@unflores unflores deleted the shell-imports branch March 10, 2025 09:40
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.

3 participants