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.

Sign up for GitHub

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

Jump to bottom

Export DGFIP authorization requests #591

Merged
merged 10 commits into from
Dec 13, 2024
Merged

Export DGFIP authorization requests #591

merged 10 commits into from
Dec 13, 2024

Conversation

skelz0r
Copy link
Member

@skelz0r skelz0r commented Dec 9, 2024 •
edited
Loading

Il n'y a pas de lien direct dans l'UI, ça sera /dgfip/export, on leur fournira le lien direct ça suffira imo.

Closes https://linear.app/pole-api/issue/DAT-209

@skelz0r skelz0r self-assigned this Dec 9, 2024
@skelz0r skelz0r force-pushed the features/export-excel-dgfip branch from c7f19df to fe19c07 Compare December 9, 2024 12:08
@linear Linear
Copy link

linear bot commented Dec 9, 2024

DAT-209 Instruction: Export des habilitations

@skelz0r skelz0r force-pushed the features/export-excel-dgfip branch from fe19c07 to 64df307 Compare December 9, 2024 13:57
@skelz0r
Copy link
Member Author

skelz0r commented Dec 10, 2024

Bon en vrai je ne sais pas trop ce qui est intéressant à relire.. parce que ici ça sera surtout de la méga itération au moment où on va faire la migration 🤷

Samuelfaure
Samuelfaure approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@Samuelfaure Samuelfaure left a comment

Choose a reason for hiding this comment

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

quelques chipotages mais good

app/services/dgfip_spreadsheet_generator.rb Show resolved Hide resolved
spec/services/dgfip_spreadsheet_generator_spec.rb Show resolved Hide resolved
spec/services/dgfip_spreadsheet_generator_spec.rb Outdated Show resolved Hide resolved
@skelz0r
Copy link
Member Author

skelz0r commented Dec 13, 2024

Faudra coupler ça avec l'import/migration et comparer side by side entre l'export v1 et v2 pour être "sûr" et ajuster en fonction. Le but étant d'avoir 2 spreadsheet "identique" (j'ai omis des colonnes de la v1 qui ne sont pas exploités par le script python)

JeSuisUnCaillou
JeSuisUnCaillou reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Juste quelques retours déjà, avant de plonger dans le code 👇

  • Y'a pas encore le bouton d'export dans l'interface exprès ?
  • Y'a sûrement des opti à faire sur les requêtes effectuées, par ce que j'en compte 18 par authorization_request là
  • Je ne reçoit pas le fichier, et j'ai une erreur cheloue dans mon serveur quand je tente d'acccéder à http://localhost:3000/dgfip/export depuis mon navigateur, en tant que [email protected] :

Screenshot from 2024-12-13 10-24-47

@skelz0r
Introduce DataProvider#authorization_definitions
0cec872
@skelz0r
Introduce raw_attributes_from_v1 text column
6ebc9c8 
Will be used for legacy export, and maybe later for some other things.
Not a json column, just plain old text, there's will no smart operation
with this column, only backfilling.
@skelz0r
Migration: save raw data from v1
d6ba4cf
JeSuisUnCaillou
JeSuisUnCaillou approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Globalement ok, dtf faudra itérer comme tu l'as dit.

app/controllers/dgfip/export_controller.rb Outdated Show resolved Hide resolved
spec/services/dgfip_spreadsheet_generator_spec.rb Outdated Show resolved Hide resolved
app/services/dgfip_spreadsheet_generator.rb Outdated Show resolved Hide resolved
@skelz0r skelz0r force-pushed the features/export-excel-dgfip branch from 64df307 to 3e5fbe1 Compare December 13, 2024 11:46
@skelz0r
Copy link
Member Author

skelz0r commented Dec 13, 2024

@JeSuisUnCaillou les 3 derniers commits

JeSuisUnCaillou
JeSuisUnCaillou reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Tu peux simplifier dgfip_reporter? et tej users_for_roles je pense.

app/models/data_provider.rb Show resolved Hide resolved
app/controllers/dgfip/export_controller.rb Show resolved Hide resolved
@skelz0r
Copy link
Member Author

skelz0r commented Dec 13, 2024

Et du coup.. je fais quoi ? Je laisse comme ça ou je fais une policy qui c/c ce qu'il y avait dans le controller ?
Et après, vu que c'est jetable comme code, je supprime la policy et je le remet dans le controller ? 🙃

@skelz0r
DGFIPSpreadsheetGenerator: harden extraction of previous_enrollment_id
ad101cd 
There is multiple cases from v1:

1. sandbox -> try to extract FC id
2. no stage -> try to extract FC id
3. unique (editeur) -> try to extract FC id
4. production -> sandbox id

Stage of 3. is production in v2, but there is no sandbox authorization,
so we fallback to a potential FC id.
@skelz0r skelz0r force-pushed the features/export-excel-dgfip branch from 3e5fbe1 to ad101cd Compare December 13, 2024 15:07
JeSuisUnCaillou
JeSuisUnCaillou requested changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ah oui en fait y'a encore ça 👇

  • Y'a sûrement des opti à faire sur les requêtes effectuées, par ce que j'en compte 18 par authorization_request là
  • Je ne reçoit pas le fichier, et j'ai une erreur cheloue dans mon serveur quand je tente d'acccéder à http://localhost:3000/dgfip/export depuis mon navigateur, en tant que [email protected] :

Screenshot from 2024-12-13 10-24-47

EDIT : c'est juste un warning en fait ça me DL bien le fichier 👍

@JeSuisUnCaillou
Copy link
Contributor

Et du coup.. je fais quoi ? Je laisse comme ça ou je fais une policy qui c/c ce qu'il y avait dans le controller ? Et après, vu que c'est jetable comme code, je supprime la policy et je le remet dans le controller ? 🙃

Vu à l'oral, on est ok tel quel (j'avais pas pigé qu'il fallait checker plusieurs auth_definitions par user)

JeSuisUnCaillou
JeSuisUnCaillou approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Reste juste à opti les requêtes, mais on peut voir plus tard.

@skelz0r skelz0r merged commit cb1c236 into develop Dec 13, 2024
13 checks passed
@skelz0r skelz0r deleted the features/export-excel-dgfip branch December 13, 2024 15:32
@skelz0r
Copy link
Member Author

skelz0r commented Dec 13, 2024

Reste juste à opti les requêtes, mais on peut voir plus tard.

En effet, quand on aura des real lifes data on mettra les bons includes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Samuelfaure Samuelfaure Samuelfaure approved these changes

@JeSuisUnCaillou JeSuisUnCaillou JeSuisUnCaillou approved these changes

@jbfeldis jbfeldis Awaiting requested review from jbfeldis

@Isalafont Isalafont Awaiting requested review from Isalafont

Assignees

@skelz0r skelz0r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skelz0r @JeSuisUnCaillou @Samuelfaure