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

[FEATURE] améliore les messages d'erreur d'import de prescrits #10814

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

lionelB
Copy link
Member

@lionelB lionelB commented Dec 13, 2024

🎄 Problème

Lorsqu'une erreur d'import survient, actuellement on a : “Merci de corriger les erreurs et d’importer à nouveau le fichier”
mais il existe des cas ou il n'y pas d'erreurs à corriger

🎁 Proposition

Dans les cas où il n'y a pas d'erreurs à corriger, on affiche “Une erreur est survenue, merci d'importer plus tard ou d'écrire au support Pix”

🧦 Remarques

ras

🎅 Pour tester

  • (non régression) Faire un import. de prescrit avec un fichier pourri
  • voir les erreur à corriger (et le lien qui mène aux erreurs)
  • Modifier les variable d'env pour mettre en pause les imports
  • Faire un import de prescrit sur orga
  • Se connecter sur S3 et supprimer le fichier (bucket de dev)
  • Modifier les variable d'env pour relancer les imports
    (et peut être lancer start:job)
  • recharger la page coté orga
  • voir le bon fichier d'erreur (sans le lien)

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@lionelB lionelB changed the title [FEATURE] améliore les message d'erreur d'import de prescrit [FEATURE] améliore les messages d'erreur d'import de prescrit Dec 13, 2024
@lionelB lionelB changed the title [FEATURE] améliore les messages d'erreur d'import de prescrit [FEATURE] améliore les messages d'erreur d'import de prescrits Dec 13, 2024
@lionelB lionelB force-pushed the pix-15551/update-import-error-message branch 2 times, most recently from d28ec86 to 9afd4ee Compare December 13, 2024 15:20
Comment on lines 14 to 16
get hasFixableError() {
return this.hasError && this.errors.some((error) => error.code || /aggregate/i.test(error.name));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce que les fixable error ne sont pas seulement les erreurs du domain ? le S3 etc... c'est des 500 ? on devrait mettre un message générique quand ce sont des erreurs que le domain ne gère pas non ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

edit : ok je vois pourquoi on fait ça.

  • Nettoyer les logs organization-imports aux retours users ( il n'ont pas a connaitre la stack trace )
  • Renvoyer un boolean qui permet de savoir si l'erreur est gérable par l'utilisateur

Copy link
Member Author

Choose a reason for hiding this comment

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

c'est fait :)

Comment on lines 253 to 255
name: 'OrganizationLearnersCouldNotBeSavedError',
stack: 'OrganizationLearnersCouldNotBeSavedError: An error occurred during process',
message: 'An error occurred during process',
Copy link
Contributor

Choose a reason for hiding this comment

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

question

Je ne suis pas sûr que côté Front on remonte ça ? si ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ça sent le smell si le front connaît la stack trace du Back. peut être qu'on devrait faire péter la route en le get organizationImportDetail, pour dire qu'il y a une erreur non gérable par l'utilisateur ?

@lionelB lionelB force-pushed the pix-15551/update-import-error-message branch from 9afd4ee to 898a1e9 Compare December 16, 2024 16:38
@lionelB lionelB requested a review from a team as a code owner December 16, 2024 16:38
@lionelB lionelB force-pushed the pix-15551/update-import-error-message branch from 898a1e9 to 33bae6d Compare December 16, 2024 16:43
attributes: ['id', 'status', 'errors', 'createdBy', 'createdAt', 'updatedAt'],
attributes: ['id', 'status', 'errors', 'hasFixableErrors', 'createdBy', 'createdAt', 'updatedAt'],
transform: function (record) {
record.errors = record.errors.map((error) => omit(error, 'stack'));
Copy link
Contributor

Choose a reason for hiding this comment

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

question/suggestion est ce que c'est pas plutôt au model de renvoyer un getter avec cette gestion ?

Comme ça le serializers fait un simple mapping ?

Copy link
Contributor

@xav-car xav-car left a comment

Choose a reason for hiding this comment

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

tech ok, c'est beaucoup plus zoli ❤️

@xav-car
Copy link
Contributor

xav-car commented Dec 18, 2024

Func KO

image

{"level":50,"time":1734516513775,"pid":21,"hostname":"pix-api-review-pr10814-web-1","tags":["internal","implementation","error"],"err":{"type":"TypeError","message":"Cannot read properties of null (reading 'map')","stack":"TypeError: Cannot read properties of null (reading 'map')\n at Object.transform (file:///app/src/prescription/learner-management/infrastructure/serializers/jsonapi/organization-import-detail-serializer.js:10:37)\n at module.exports.perform (/app/node_modules/jsonapi-serializer/lib/serializer-utils.js:273:21)\n at resource (/app/node_modules/jsonapi-serializer/lib/serializer.js:38:33)\n at module.exports.serialize (/app/node_modules/jsonapi-serializer/lib/serializer.js:60:14)\n at Module.serialize (file:///app/src/prescription/learner-management/infrastructure/serializers/jsonapi/organization-import-detail-serializer.js:14:6)\n at getOrganizationImportStatus (file:///app/src/prescription/learner-management/application/organization-import-controller.js:11:69)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n at async exports.Manager.execute (/app/node_modules/@hapi/hapi/lib/toolkit.js:60:28)\n at async internals.handler (/app/node_modules/@hapi/hapi/lib/handler.js:46:20)\n at async exports.execute (/app/node_modules/@hapi/hapi/lib/handler.js:31:20)\n at async Request._lifecycle (/app/node_modules/@hapi/hapi/lib/request.js:370:32)\n at async Request._execute (/app/node_modules/@hapi/hapi/lib/request.js:280:9)","isBoom":true,"isServer":true,"data":null,"output":{"statusCode":500,"payload":

a priori quand il n'y a pas d'erreur. ça marche pas :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants