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(transfer-alert): improve validation #496

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

balzdur
Copy link
Collaborator

@balzdur balzdur commented Jul 5, 2024

Small changes :

  • better schema (only move optionnal + max length, based on what we discussed I didn't implemented a "smart" schema on IBAN or Transfer end2end id requirement)
  • consistent ordering based on the same order used in the AlertData component
  • small fixes on the adapters to allow the possibility to remove an optionnal field (with the current optionnal implem on the API, a missing field is equivalent to no PATCH changes, so we need to explicitly inform of removed fields)

NB: in fine, in a lot of places, I tends to inject "nullable values" for all missing fields to enforce the possibility to remove a field. Finally, I do more or less a PUT every time... not a big deal but if you have a better idea on how do deal with PATCH + "delete some fileds" tell me your idea

@Pascal-Delange
Copy link
Contributor

NB: in fine, in a lot of places, I tends to inject "nullable values" for all missing fields to enforce the possibility to remove a field. Finally, I do more or less a PUT every time... not a big deal but if you have a better idea on how do deal with PATCH + "delete some fileds" tell me your idea

Yep I'm afraid I don't have a silver bullet either... IMO the best workable solution is a nullable input where null input = ignore it, empty string input = reset it

@balzdur balzdur merged commit cbff63f into main Jul 5, 2024
3 checks passed
@balzdur balzdur deleted the thomas/small-alert-changes branch July 5, 2024 12:23
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