Make period_type mandatory for MembershipType #18395
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
|
style issue |
409e699 to
5748e72
Compare
|
Hmm - the failure changes with #18365 merged as the test no longer uses the same setup - I will see if I can get that merged first |
|
@eileenmcnaughton Test failures relate |
|
I'm going to close this for now because it's blocked until I can resolve the test failures - but it needs #18365 I don't want to clog up the review queue with PRs that are not reviewable so I'll re-open this when the blocking-issues are solved |
|
@eileenmcnaughton needs rebase |
5748e72 to
6aee147
Compare
|
@colemanw rebased now - let's see how it goes.... |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Sep 9, 2020
This converts the test set up to use the api and also does some mild date clean up on the form. Supports civicrm#18395
|
OK - this cleanup should address #18413 |
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Sep 9, 2020
This converts the test set up to use the api and also does some mild date clean up on the form. Supports civicrm#18395
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Sep 9, 2020
This converts the test set up to use the api and also does some mild date clean up on the form. Supports civicrm#18395
eileenmcnaughton
added a commit
to eileenmcnaughton/civicrm-core
that referenced
this pull request
Sep 9, 2020
This converts the test set up to use the api and also does some mild date clean up on the form. Supports civicrm#18395
We have a test ensuring period_type is mandatory at the api level. However, it isn't for api v4 - the error is a different issue (handling of pseudoconstants when mapping v3 to v4). This fixes that separate issue & declares required to re-fix for v4
6aee147 to
0524826
Compare
|
This is passing now |
|
Looks fine to me merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
We have a test ensuring period_type is mandatory at the api level. However, it isn't for api
v4 - the error is a different issue (handling of pseudoconstants when mapping v3 to v4). This
fixes that separate issue & declares required to re-fix for v4
Before
period_type not declared as mandatory (but has a db default of rolling). In apiv3 & form layer it is mandatory, but not apiv4
After
period_type declared as mandatory (but has a db default of rolling), required in apiv3, v4, form layer
Technical Details
Test came from https://issues.civicrm.org/jira/browse/CRM-20010 - I think re-reading this it is probably right to make it mandatory & use the default less
Comments
Came from test issues in #18388