[ODS-5665] Add ability to query organizations by an identification code #1166
Conversation
mjaksn
force-pushed
the
ODS-5665
branch
8 times, most recently
from
d3c114a to
ed5f800
Compare
| private const string IdentificationCodeTableAlias = "idct"; | ||
| private readonly ConcurrentDictionary<FullName, Join> _identificationCodeTableJoinByRootEntityName = new(); | ||
|
|
||
| public IdentificationCodeAggregateQueryCriteriaApplicator( |
There was a problem hiding this comment.
Naming conventions would indicate a name of IdentificationCodeAggregateRootQueryCriteriaApplicator here. See # 4 in Naming Conventions .
| { | ||
| if (additionalParameters == null || !additionalParameters.Any() || additionalParameters.All( | ||
| ap => AggregateRootCriteriaProviderHelpers.PropertiesToIgnore.Contains(ap.Key, StringComparer.OrdinalIgnoreCase))) | ||
| return; |
There was a problem hiding this comment.
Coding conventions call for these return statements to be wrapped in { and }. Current state of the documentation isn't explicit on this point, but it is heavily implied by the Formatting section, and this would be inconsistent with the rest of the code base.
| // If the entity does not have an identificationCodes collection with queryable properties, return | ||
| if (!_resourceIdentificationCodePropertiesProvider.TryGetIdentificationCodeProperties( | ||
| resource, out List<ResourceProperty> identificationCodeProperties)) | ||
| return; |
There was a problem hiding this comment.
Coding conventions call for these return statements to be wrapped in { and }.
| .ToArray(); | ||
|
|
||
| if (applicableAdditionalParameters.Length == 0) | ||
| return; |
There was a problem hiding this comment.
Development standards call for these return statements to be wrapped in { and }.
| // Find any supplied additionalParameters with a non-default value and name matching that of a queryable identificationCode property, if none then return | ||
| var applicableAdditionalParameters = additionalParameters | ||
| .Where( | ||
| x => !x.Value.IsDefaultValue() && |
There was a problem hiding this comment.
Coding conventions call for logical operators like && and || to be placed as leading on the next line, not trailing on the previous line.
| _provider = new ResourceIdentificationCodePropertiesProvider(); | ||
| const string ResourceName = "edfi.Calendar"; | ||
|
|
||
| var domainModel = DomainModelDefinitionsProviderHelper.DomainModelProvider.GetDomainModel(); |
There was a problem hiding this comment.
Maybe put this in a [OneTimeSetUp] attributed method since it's the same model every time.
| } | ||
|
|
||
| [Test] | ||
| public void TryFindIdentificationCodes_ShouldReturnTrueAndCorrectIdentificationCodeProperties_WhenABaseResourceHasIdentificationCode() |
There was a problem hiding this comment.
Maybe rename this to remove "Base" from the final name segment? Course is not really a base class, and it fits with the test above which is testing WhenResourceHasNoIdentificationCode -- maybe just use WhenResourceHasIdentificationCode?
| } | ||
|
|
||
| [Test] | ||
| public void TryFindIdentificationCodes_ShouldReturnTrueAndCorrectIdentificationCodeProperties_WhenADerivedResourceHasIdentificationCode() |
There was a problem hiding this comment.
Definitely optional ... you could go with WhenADerivedResourceHasInheritedIdentificationCode here.
| "method": "GET", | ||
| "header": [], | ||
| "url": { | ||
| "raw": "{{ApiBaseUrl}}/data/v3/ed-fi/localEducationAgencies?educationOrganizationIdentificationSystemDescriptor=uri://ed-fi.org/EducationOrganizationIdentificationSystemDescriptor%23non-existatnt", |
There was a problem hiding this comment.
Correct the spelling on non-existent...
| @@ -1950,6 +1952,206 @@ | |||
| "response": [] | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "name": "When filtering by IdentificationCode", | |||
There was a problem hiding this comment.
I feel like these tests could really use some negative test data (data that would show up in the results if the implementation was incorrect). For example, in a "Setup" folder create a new temporary School that has identification codes that match the value being searched but uses a different identification system. (Also add a "Teardown" folder that removes this School and use a separate subfolder in between to hold the test requests, as necessary).
So, for example, create a School with an identification code that uses uri://ed-fi.org/EducationOrganizationIdentificationSystemDescriptor#Other with a value of 255901001. This school should also be returned when only searching by identification code 255901001 but should not be returned if both the identification system and code are used in the search.
…terface implementation
…ntificationCode property Postman test
…ambiguous column name
| parameterList.Add( | ||
| new Parameter | ||
| { | ||
| name = (x.DescriptorName ?? x.PropertyName).ToCamelCase(), |
There was a problem hiding this comment.
So, I'm curious about the decision to use DescriptorName instead of the PropertyName here. The DescriptorName property actually returns the name of the descriptor entity and will in many cases be the same as the property name, but in the event that a role-name is applied, this will have the effect of stripping that. Is that what was intended here?
There was a problem hiding this comment.
I was thinking of entities and concerned descriptor property names could be suffixed with Id. That's not an issue with resources, so I changed it to only use PropertyName in the latest commit.
| @@ -0,0 +1,98 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
Fix typo in the filename.
No description provided.