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

Enhance validation on send application mutation #1472

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matti-lamppu
Copy link
Collaborator

@matti-lamppu matti-lamppu commented Dec 18, 2024

🛠️ Changelog

  • Validate that applications are in a good state before allowing them to be sent

🧪 Test plan

  • Automated tests

🚧 Dependencies

  • None

🎫 Tickets

@matti-lamppu matti-lamppu added the improvement Improves an existing feature label Dec 18, 2024
@matti-lamppu matti-lamppu self-assigned this Dec 18, 2024
@matti-lamppu matti-lamppu force-pushed the validate-application-section branch from dd44f1c to 56ff3f2 Compare December 18, 2024 09:48
@matti-lamppu matti-lamppu marked this pull request as ready for review December 18, 2024 11:06
msg = "Application organisation must have a core business."
errors[api_settings.NON_FIELD_ERRORS_KEY].append(msg)

if OrganizationTypeChoice(organisation.organisation_type) not in allowed_organisation_types:
Copy link

Choose a reason for hiding this comment

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

organisation_type is no longer saved for applications, so can't be validated
e.g. https://tilavaraus.test.hel.ninja/admin/tilavarauspalvelu/application/1053/change/ is an application for COMMUNITY but its org.type is the default of COMPANY

Copy link

@vergama vergama left a comment

Choose a reason for hiding this comment

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

Looks fine except that the deprecated organisation_type can no longer be validated for a value matching the applicant type. (Probably can't be just migrated away if the values for old applications are wanted for stats, although an empty default would now be better...)

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

Successfully merging this pull request may close these issues.

2 participants