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

fix(dealers): exclude unwanted fields from API #194

Merged
merged 3 commits into from
Sep 1, 2024

Conversation

Fenrikur
Copy link
Member

@Fenrikur Fenrikur commented Aug 31, 2024

  • AttendeeNickname should only be exposed via DisplayNameOrAttendeeNickname if DisplayName is not set.
  • RegistrationNumber should never be exposed.

@Fenrikur Fenrikur requested review from Rain336 and Metawolve August 31, 2024 12:51
@Fenrikur Fenrikur self-assigned this Aug 31, 2024
@Fenrikur Fenrikur changed the title fix(dealers): exclude unnecessary fields from API fix(dealers): exclude unwanted fields from API Aug 31, 2024
@Fenrikur Fenrikur requested a review from maakinoh August 31, 2024 18:13
Copy link
Contributor

@maakinoh maakinoh left a comment

Choose a reason for hiding this comment

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

JsonIgnore will result in an database when creating new dealer entries error since both fields are still set as Required.

Therefore, the POST API endpoint /Api/Dealers cant be used anymore for creating new dealer entries.

Since dealers are created in a separate job this should still be fine to merge but the POST endpoint /Api/Dealers could be documented accordingly

@Fenrikur
Copy link
Member Author

Fenrikur commented Sep 1, 2024

JsonIgnore will result in an database when creating new dealer entries error since both fields are still set as Required.

Therefore, the POST API endpoint /Api/Dealers cant be used anymore for creating new dealer entries.

Since dealers are created in a separate job this should still be fine to merge but the POST endpoint /Api/Dealers could be documented accordingly

Thank you for noticing! Didn't think about that.
The API in question is currently not being used and will have to undergo some changes for its intended use in EF29 anyways, so this should not be much of a problem. Dealers are currently created by an import job directly accessing the services internally.
I've added TODO comments and temporarily set default values for the two required fields that are no longer exposed on the API just to not break things though. (=

@Fenrikur Fenrikur requested a review from maakinoh September 1, 2024 13:32
@Fenrikur Fenrikur merged commit c1d8232 into main Sep 1, 2024
@Fenrikur Fenrikur deleted the bugfix/exclude-dealer-fields branch September 1, 2024 14:04
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