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

Filter current dataset subindicators #276

Merged
merged 5 commits into from
May 31, 2021

Conversation

michaelglenister
Copy link
Member

@michaelglenister michaelglenister commented May 27, 2021

This will only fetch/create sub-indicators for filter values from the current dataset

Description

Related Issue

OpenUpSA/wazimap-ng-ui#346
https://trello.com/c/SWt9HuT3/733-productionise-vega-and-data-branch

How to test it locally

Upload a dataset and check if group ordering contains unexpected filter values

Changelog

Added

Updated

Removed

Checklist

  • 🚀 is the code ready to be merged and go live?
  • 🛠 does it work (build) locally

Pull Request

  • 📰 good title
  • 📝good description
  • 🔖 issue linked
  • 📖 changelog filled out

Commits

  • commits are clean
  • commit messages are clean

Code Quality

  • 🚧 no commented out code
  • 🖨 no unnecessary logging
  • 🎱 no magic numbers
  • black was run locally (as part of the pre-commit hook)

Testing

  • ✅ added (appropriate) unit tests
  • 💢 edge cases in tests were considered
  • ✅ ran tests locally & are passing

This will only fetch/create subindicators for filter values from the
current dataset
@milafrerichs milafrerichs temporarily deployed to wazimap-fix-multiple-ca-g4mnf6 May 27, 2021 13:43 Inactive
Test if new dataset will not use subindicators from other datasets
@milafrerichs milafrerichs temporarily deployed to wazimap-fix-multiple-ca-ruezta May 28, 2021 12:54 Inactive
Copy link
Contributor

@milafrerichs milafrerichs left a comment

Choose a reason for hiding this comment

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

can we add another test where the new dataset get's another "upload" so another datasetdata and then the groups will have both subindicators but not from the other datasets?

@@ -263,3 +263,19 @@ def test_subindicators_update(self, dataset, datasetData, subindicatorGroup):
assert len(groups) == 2
assert groups[0].subindicators == ["A", "B", "C"]
assert groups[1].subindicators == ["X", "Y"]

def test_new_dataset(self, dataset, datasetData, subindicatorGroup):
dataset = DatasetFactory()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename dataset to something else like new_dataset just to make it clear this is a different dataset then the fixture.

Copy link
Collaborator

@goyal1092 goyal1092 left a comment

Choose a reason for hiding this comment

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

@michaelglenister dataloader code looks good to me but i think test needs one more dataset and DatasetData object to test actual results.

In this test we should be testing what happens if there are DatasetData objects that belongs to different datasets but have same group with diff values.

Previously it was merging subindicator values in respective to the dataset.

Add comment to explain unit test and rename the method
@milafrerichs milafrerichs temporarily deployed to wazimap-fix-multiple-ca-xhy4na May 28, 2021 14:27 Inactive
Add checks to see if the previous dataset is effected after adding a new
dataset
@milafrerichs milafrerichs temporarily deployed to wazimap-fix-multiple-ca-2n4srg May 31, 2021 11:21 Inactive
@michaelglenister
Copy link
Member Author

I added additional checks to test that subindicators are not leaking between datasets using the same group names

Copy link
Contributor

@milafrerichs milafrerichs left a comment

Choose a reason for hiding this comment

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

almost there :)


assert len(groups) == 2
assert groups[0].subindicators == ["D"]
assert groups[0].subindicators != ["A", "B", "C", "D"]
Copy link
Contributor

Choose a reason for hiding this comment

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

not quite right.

datasetData creates two subindicators A and B but not C so this will never be the case.

def test_old_dataset_does_not_include_new_subindicators(self, dataset, datasetData, subindicatorGroup):
# Make sure same group names from a previous dataset does not have subindicators leaking into it

DatasetDataFactory(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not the right test, this does not create a new dataset.

You should do the same as in above test:

new_dataset = DatasetFactory()
DatasetDataFactory(dataset=new_dataset, data={'group1': 'D', 'group2': 'Z' } )

otherwise D and Z will never be there and cannot leak into the old group with the same name.

Fixed remaining issues with the new test cases
@milafrerichs milafrerichs temporarily deployed to wazimap-fix-multiple-ca-imoqet May 31, 2021 12:56 Inactive
@milafrerichs milafrerichs requested a review from goyal1092 May 31, 2021 14:15
@milafrerichs milafrerichs dismissed goyal1092’s stale review May 31, 2021 14:16

the tests were updated according to your review and mine.

@milafrerichs milafrerichs merged commit 1ce6ec9 into staging May 31, 2021
@adieyal adieyal deleted the fix-multiple-capitalisations branch August 6, 2021 10:11
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.

3 participants