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

Implements #611 -- Customizable location and relationship types #1429

Merged
merged 9 commits into from
May 30, 2017

Conversation

oliverroick
Copy link
Member

@oliverroick oliverroick commented Apr 25, 2017

Proposed changes in this pull request

This PR adds changes required to implement #611. The basic idea is that project managers can customise location and relationship types via the location_type or tenure_type fields in a questionnaire. Only types defined in the questionnaire will be allowed. If the questionnaire does not define a question, standards types will be used.

core

  • Adds core.form_mixins.get_types which returns a list of valid types of for a select field. It expects a question name (either tenure_type or location_type) and returns all possible options if the question is defined for the given questionnaire ID. If no questionnaire ID is given or the question is not defined in the questionnaire, default is returned. There's an addition option include labels for all choices.

organization

  • Two additional settings need to be added to the import config: allowed_location_types and allowed_tenure_types. Both are generated using core.form_mixins.get_types. They define the accepted types for the project. Imported tenure types and location types are validated against these lists.
  • The fields tenure_type.id and tenure_type.label were renamed to tenure_type_id and tenure_type_label for imports and exports.
  • Adapted the following test files to the new import/export schema:
    • cadasta/organization/tests/files/test_conditionals.xlsx
    • cadasta/organization/tests/files/test_download.xlsx
    • cadasta/organization/tests/files/test_one_to_many.xlsx

party

  • I removed to TenureRelationshipType model and turned the tenure_type field of the Party model into a simple CharField. We originally implemented the TenureRelationshipType in anticipation that we are going to implement custom tenure types. I decided not to use TenureRelationshipType because we would have to store information about valid selections for tenure types in two places: TenureRelationshipType and Questionnaire. Instead, I'm only storing that information with the Questionnaire and feed all forms, serializers, imports, exports from there.
  • Moves party.models.TENURE_RELATIONSHIP_TYPES to party.choices.TENURE_RELATIONSHIP_TYPES to be consistent with other app.
  • If a project doesn't have a questionnaire, valid options for tenure_type are defined via party.choices.TENURE_RELATIONSHIP_TYPES.
  • Removing TenureRelationshipType and falling back to simple CharField for tenure_type has the following implications:
    • Search: TenureRelationships are indexed including the field tenure_type_id, which isn't present any longer. This should be changed to tenure_type.
  • Adds validate_tenure_type to party.serializers.TenureRelationshipWriteSerializer to make sure tenure_types are validated via the API.
  • Changes the method tenure_type_label of party.models.TenureRelationship to return the correct label for a TenureRelationship.
  • Removes all references to the loadtenurereltypes command as it is no longer relevant.
  • Removes ACQUIRED_CHOICES from party.models.TenureRelationship because it's not used anywhere.

spatial

  • Remove the static choices parameter from spatial.models.SpatialUnit so accepted choices for the field are customisable.
  • Adds location_type_label to spatial.models.SpatialUnit to return the correct label for the location type.
  • Adds validate_type to spatial.serializers.SpatialUnitSerializer to make sure location_types are validated via the API.

When should this PR be merged

  • whenever accepted

Risks

  • The PR contains migrations, which are not complex but should be tested against demo and production.

Follow-up actions

  • Search index needs to be updated to reflect the changes in tenure type handling.

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
  • Is the PR labeled correctly? It should have the migration label if a new migration is added.
  • Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • Are all UI and API inputs run through forms or serializers?
  • Are all external inputs validated and sanitized appropriately?
  • Does all branching logic have a default case?
  • Does this solution handle outliers and edge cases gracefully?
  • Are all external communications secured and restricted to SSL?

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

@amplifi
Copy link
Contributor

amplifi commented Apr 25, 2017

@oliverroick I can modify the indexing as needed to match the new schema.

@seav
Copy link
Contributor

seav commented Apr 25, 2017

Regarding the export format, why are we including the tenure type label in the first place? We don't​ do the same for the location or party type or for any select_one or select_multiple questions in the questionnaire.

@oliverroick oliverroick force-pushed the feature/custom-types branch from fd10811 to 83dd095 Compare April 28, 2017 08:55
@oliverroick oliverroick requested review from alukach and bjohare April 28, 2017 10:36
@oliverroick oliverroick force-pushed the feature/custom-types branch from 83dd095 to 263216a Compare April 28, 2017 11:07
@oliverroick oliverroick changed the title Feature/custom types Implements #611 -- Customizable location and relationship types Apr 28, 2017
Copy link
Contributor

@alukach alukach left a comment

Choose a reason for hiding this comment

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

The code looks good except for some time things. Not sure if I fully understand the implications of the feature, maybe we can do a quick demo?

lang = get_language()
default_lang = question.questionnaire.default_language

for o in options:
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of clarity, can we iterate as for name, label in options: ?

except Question.DoesNotExist:
pass
else:
types = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary redefinition of types from line 15.

if include_labels:
return types
else:
return [t[0] for t in types]
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, for legibility can we call do something like [name for name, label in types] so that it's more obvious what will be put in the array?

migrations.DeleteModel(
name='HistoricalTenureRelationshipType',
),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason that we can't merge this with 0003_convert_tenuretype_to_charfield?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I merged both

@oliverroick oliverroick requested review from linzjax and removed request for bjohare May 3, 2017 16:31
Copy link
Contributor

@linzjax linzjax left a comment

Choose a reason for hiding this comment

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

Couple of broken things.

Just to confirm: when uploading a questionnaire, you still need to have labels for the choices? I think I'm confused by the get_types tests that test not including labels.

@@ -113,7 +117,65 @@ def test_adding_attributes(self):

def test_name(self):
su = SpatialUnitFactory.create(type="RW")
assert su.name == "Right-of-way"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this test if it's supposed to test the name property? Shouldn't name and location_type_label return the same thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I first folded the functionality that is now in location_type_label into name but later decided that it should live in location_type_label so it's consistent with TenureRelationship.

I'll revert that so the test is for location_type_label

Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore that. name the test is for name

@@ -51,16 +49,16 @@ def validate_row(headers, row, config):

if location_type_field:
location_type = _get_field_value(location_type_field, "location_type")
type_choices = [choice[0] for choice in TYPE_CHOICES]
type_choices = config['allowed_location_types']
if location_type and location_type not in type_choices:
raise ValidationError(
_("Invalid location_type: '%s'.") % location_type
Copy link
Contributor

Choose a reason for hiding this comment

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

I entered a questionnaire with 'DR' as a location type choice, and it works in the UI. However, when I tried to import data, this error was raised saying 'DR' was an invalid location_type.

It didn't raise when I used one of the default location_type options, and it should have since I entered a custom quesitonnaire. Instead, the platform errored out when trying to load the locations.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 That's fixed now.

@@ -172,12 +182,7 @@ def get_main_label(self, model, source):
if model == SpatialUnit:
return spatial_type_choices.get(source['type'], '—')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be updated somewhere? If it's a custom type it's only returning '-'. The '-' should be 'Their land' (as shown in the relationship search result).

screen shot 2017-05-06 at 12 55 36 pm

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I understand. Where is "Their Land" coming from? Is it defined in the Questionnaire?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's the label defined in the questionnaire.

Copy link
Member Author

Choose a reason for hiding this comment

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

The custom labels weren't used in the search view. That's sorted now.

def validate_tenure_type(self, value):
prj = self.context['project']
allowed_types = get_types('tenure_types',
TENURE_RELATIONSHIP_TYPES,
Copy link
Contributor

Choose a reason for hiding this comment

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

This only allows default tenure choices to be used, so I can't update any custom types through the API:

{
    "tenure_type": [
        "'FU' is not a valid choice for field 'tenure_type'."
    ]
}

But if you query to tenure record, the type is set to FU, which is allowed...

{
    "attributes": {},
    "id": "8biusfwafn7zjnkphapjqutp",
    "party": {
        "id": "zgvyctdtxybxjri5upw2g9e4",
        "name": "Fuzzy Wuzzy",
        "type": "IN"
    },
    "rel_class": "tenure",
    "spatial_unit": {
        "geometry": {
            "coordinates": [
                131.02294921875,
                31.484893386890164
            ],
            "type": "Point"
        },
        "properties": {
            "id": "p5sgtrjezgjsu7ujwdb6exeb",
            "type": "TL"
        },
        "type": "Feature"
    },
    "tenure_type": "FU"
}

It works for custom location types

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this should read allowed_types = get_types('tenure_type',. Added tests as well.

@oliverroick
Copy link
Member Author

Sooo. I addressed the requests.

There's probably more work to be done, now that the search export is merged... Going to look into that this week.

@oliverroick oliverroick force-pushed the feature/custom-types branch from 5460f1a to 6a3ffa3 Compare May 10, 2017 13:29
Copy link
Contributor

@linzjax linzjax left a comment

Choose a reason for hiding this comment

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

This looks good to me. One comment about (I think) leftover code. Do we need to reassess the last commit now that search export is being reverted?

include_labels=True)
self.spatial_types = dict(spatial_types)

SPATIAL_TYPE_CHOICES
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this serving a purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

I obviously left it in there on purpose — the check whether you review carefully ;) Looks like you do.

It's removed now.

@seav
Copy link
Contributor

seav commented May 12, 2017

Just reiterating this question again. Regarding the export format, why are we including the tenure type label in the first place? We don't​ do the same for the location or party type or for any select_one or select_multiple questions in the questionnaire. I think it would make things slightly simpler if we remove labels from the export.

@oliverroick
Copy link
Member Author

I agree that exporting the labels doesn't serve a particular purpose. I'd vouch for removing it from all exports. @dpalomino, what's your take on this?

If we decide to keep the labels, then we should be consistent and add the labels everywhere.

@dpalomino
Copy link

I agree that exporting the labels doesn't serve a particular purpose. I'd vouch for removing it from all exports. @dpalomino, what's your take on this?

Thanks @oliverroick. Yes, I agree. I think with the tenure_type id and the location_type id should work.

@oliverroick oliverroick force-pushed the feature/custom-types branch from 393f556 to 0ab2bdd Compare May 30, 2017 10:52
@amplifi amplifi merged commit 4ba5b25 into master May 30, 2017
@oliverroick oliverroick deleted the feature/custom-types branch May 31, 2017 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants