Generic Merge #757

BarisSari · 2021-02-09T15:50:57Z

🧰 Issue

Fixes #754

🔨Work carried out:

Allow merging Tasks and Tags
Allow merging reference table objects
Allow merging Sensors and Datafiles (by moving measurements)
Tests pass

I also fixed the Sphinx error, which was causing failure: sphinx-doc/sphinx#8885

Confirmations

I have chosen reviewers for my PR.
I have chosen an appropriate label for the PR, adding interactive_review if reviewers will need to see UI
I have extended/updated the documentation in \docs folder
Any database changes are recorded in the Log/Changes tables
I have completed the mandatory sections of this document.
I have deleted any unused sections.

📝 Developer Notes:

Ian suggested writing a test that merges nations, but I thought it would be better to use the most used reference table: Privacies. That's why I tested merge methods with it.

While I was working to find out the referencing foreign key fields of a table, I found sqlalchemy-utils. And I realised that I'm trying to reinvent the wheel. My function was almost identical to one of its helper functions. 😄 So, I also included sqlalchemy-utils in this PR. It has many useful functions, especially foreign key helpers: https://sqlalchemy-utils.readthedocs.io/en/latest/foreign_key_helpers.html .

You might ask why I didn't use merge_references(from_obj, to_obj) for all merge-operations. The reason is that we should handle dependent objects(Platform-Sensor, etc.) differently. If I tried to merge two platforms that have the same sensor name, it would throw a constraint error. So, I tried to use it as much as I can (for merging reference table objects), but I didn't use it for special cases.

codecov · 2021-02-09T16:17:55Z

Codecov Report

Merging #757 (7d07b5a) into develop (36b0306) will increase coverage by 0.04%.
The diff coverage is 97.10%.

@@             Coverage Diff             @@
##           develop     #757      +/-   ##
===========================================
+ Coverage    90.98%   91.03%   +0.04%     
===========================================
  Files           84       84              
  Lines         7889     7933      +44     
===========================================
+ Hits          7178     7222      +44     
  Misses         711      711

Impacted Files	Coverage Δ
pepys_import/core/store/sqlite_db.py	`100.00% <ø> (ø)`
pepys_admin/maintenance/gui.py	`58.46% <50.00%> (+0.16%)`	⬆️
pepys_import/core/store/data_store.py	`98.92% <98.48%> (+0.07%)`	⬆️
pepys_import/core/store/common_db.py	`99.42% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9e9555...8cdb3c5. Read the comment docs.

IanMayo · 2021-02-11T09:44:37Z

So, I tried to use it as much as I can (for merging reference table objects), but I didn't use it for special cases.

Sounds perfect to me, @BarisSari :-)

migrations/sqlite_versions/2021-02-10_a382a0f4b1be_correct_privacy_id_type_of_hostedby.py

…c_merge # Conflicts: # .isort.cfg # requirements.txt

pepys_import/core/store/data_store.py

IanMayo · 2021-02-11T13:03:00Z

I'm sure I made this comment before, @BarisSari - but now I can't find it.

I couldn't see this in the tests. You're testing merging entries in the Privacies table. It would be a good idea to load some State entries from file, and check their privacy gets changed to the new merged value. This will check that merge_references works in the way we're expecting it to.

BarisSari · 2021-02-11T13:09:14Z

I'm sure I made this comment before, @BarisSari - but now I can't find it.

I couldn't see this in the tests. You're testing merging entries in the Privacies table. It would be a good idea to load some State entries from file, and check their privacy gets changed to the new merged value. This will check that merge_references works in the way we're expecting it to.

Sure, @IanMayo. I'm extending the related test.

IanMayo

Great, thanks. Will wait for @robintw review.

robintw

Thanks for this @BarisSari.

The approach looks good, and I like the sqlalchemy-utils functions you're using.

I'm a little confused about a few bits though:

Can merge_generic merge platforms? It doesn't look like it can at the moment - it'd be really good if all merges could just go through the merge_generic function, so we didn't have to do platforms separately. I can see why the merge_platforms function hasn't been deleted at the moment (as it would break the GUI), but hopefully we could delete it once I'd updated the GUI to use merge_generic.
What about the other metadata tables that we might want to merge? I can see the code deals with Sensor and Datafile at the moment. What about Task, HostedBy, Participant etc. @IanMayo I assume I'm correct in saying these should be able to be merged too (I know we're not using them at the moment, but it'd be good if the code supported them for when we start using them shortly).

BarisSari · 2021-02-12T13:14:02Z

Thanks for the review, @robintw.

Yes, I didn't want to break the current flow. But, I'm going to update merge_generic. It makes sense to use it for merging Platforms as well.
I did it on purpose because Ian didn't mention those tables in the issue. According to his answer, I can update the method.

IanMayo · 2021-02-12T15:02:47Z

@BarisSari - @robintw highlighted a couple of other tables that it would make sense to support merge:

Tasks (in case two people add the same exercise, resulting in duplicates)
Tags (for removing duplicates, or when granularity was too fine. Analyst creates tags for 2020 Jan, 2020 Feb, ... But then wishes to re-group into 2020 Q1, 2020 Q2, etc).

BarisSari · 2021-02-16T08:10:58Z

@BarisSari - @robintw highlighted a couple of other tables that it would make sense to support merge:

Tasks (in case two people add the same exercise, resulting in duplicates)

Tags (for removing duplicates, or when granularity was too fine. Analyst creates tags for 2020 Jan, 2020 Feb, ... But then wishes to re-group into 2020 Q1, 2020 Q2, etc).

Okay, @IanMayo. I'm modifying the PR.

…move duplicated code from merge_platforms

…able foreign key (parent)

robintw

Thanks @BarisSari, this looks good.

I see you've replaced most calls to merge_platforms with merge_generic - but there are a couple that have been missed in the tests. Could you switch those over too?

Also, given that we've now replaced all calls to merge_platforms - I think we should be ok to delete merge_platforms (I see you've replaced the call in the GUI too).

robintw · 2021-02-17T09:26:36Z

tests/test_data_store_merge_operations.py

@@ -215,7 +218,9 @@ def test_merge_platforms_with_different_sensor_names(self):
            assert len(states_before_merge) == 0
            assert len(contacts_before_merge) == 0

-            self.store.merge_platforms([platform_2.platform_id], platform.platform_id)
+            self.store.merge_platforms(


Most of the other calls to merge_platforms seem to have been switched to merge_generic, apart from this one and a couple of others.

BarisSari · 2021-02-17T09:39:13Z

Okay, @robintw. I'll update the tests. But actually, I'm still calling the merge_platforms inside of merge_generic:
https://github.com/debrief/pepys-import/pull/757/files#diff-438b06426ef37127a7f6f793bc66ffc87ca4752f3f5f653d77fb1e9eb223e53bR1851

I just removed duplicated codes, and simplified the merge_platform. But I didn't delete it completely because merge_generic would be more complex.

robintw · 2021-02-17T11:12:59Z

Ah yes of course @BarisSari - that makes sense. I'll approve now.

robintw

All looks good now, thanks for this Baris.

BarisSari added 2 commits February 9, 2021 18:49

Implement merge_measurements method to merge Sensors and Datafiles

49d5e1d

Correct table names and objects for Datafiles, remove merged objects

f8b506f

BarisSari added 6 commits February 10, 2021 13:29

Solve synchronize_session parameter error

0a73a56

Use sqlalchemy-utils to merge reference objects

41ef867

Make privacy_id of HostedBy UUIDType instead of integer

a054ab9

Simplify new methods' parameters and bodies, write tests

cb05205

Remove duplicated code and print statement

2430da6

Make merge_measurements more readable

dad0418

BarisSari added the Task Package of work label Feb 11, 2021

BarisSari marked this pull request as ready for review February 11, 2021 09:30

BarisSari requested review from robintw and IanMayo February 11, 2021 09:30

IanMayo reviewed Feb 11, 2021

View reviewed changes

migrations/sqlite_versions/2021-02-10_a382a0f4b1be_correct_privacy_id_type_of_hostedby.py Outdated Show resolved Hide resolved

BarisSari added 2 commits February 11, 2021 13:00

Remove comments

3569295

Merge branch 'develop' of github.com:debrief/pepys-import into generi…

7ddb992

…c_merge # Conflicts: # .isort.cfg # requirements.txt

IanMayo reviewed Feb 11, 2021

View reviewed changes

pepys_import/core/store/data_store.py Show resolved Hide resolved

Merge branch 'develop' into generic_merge

43a57d5

Extend merge_object test

cc3f25f

IanMayo self-requested a review February 11, 2021 13:35

IanMayo previously approved these changes Feb 11, 2021

View reviewed changes

robintw requested changes Feb 12, 2021

View reviewed changes

Merge branch 'develop' into generic_merge

f594f33

Allow to use merge_generic for merging Platforms, Tasks, and Tags, re…

dac9d03

…move duplicated code from merge_platforms

BarisSari dismissed IanMayo’s stale review via dac9d03 February 16, 2021 08:46

BarisSari added 2 commits February 16, 2021 12:36

Write a test for merging tasks, fix TaskMixin problem because of null…

c3e0de1

…able foreign key (parent)

Fix Sphinx error

7f9f68c

BarisSari force-pushed the generic_merge branch from a265553 to 7f9f68c Compare February 16, 2021 09:37

Merge branch 'develop' into generic_merge

7491910

BarisSari requested a review from robintw February 17, 2021 07:03

robintw requested changes Feb 17, 2021

View reviewed changes

Change merge_platforms calls with merge_generic

8cdb3c5

robintw self-requested a review February 17, 2021 11:13

robintw approved these changes Feb 17, 2021

View reviewed changes

IanMayo merged commit 9b49d17 into develop Feb 17, 2021

IanMayo deleted the generic_merge branch February 17, 2021 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic Merge #757

Generic Merge #757

BarisSari commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

IanMayo commented Feb 11, 2021

IanMayo commented Feb 11, 2021

BarisSari commented Feb 11, 2021

IanMayo left a comment •

edited

Loading

robintw left a comment

BarisSari commented Feb 12, 2021

IanMayo commented Feb 12, 2021

BarisSari commented Feb 16, 2021

robintw left a comment

robintw Feb 17, 2021

BarisSari commented Feb 17, 2021

robintw commented Feb 17, 2021

robintw left a comment

Generic Merge #757

Generic Merge #757

Conversation

BarisSari commented Feb 9, 2021 • edited Loading

🧰 Issue

🔨Work carried out:

Confirmations

📝 Developer Notes:

codecov bot commented Feb 9, 2021 • edited Loading

Codecov Report

IanMayo commented Feb 11, 2021

IanMayo commented Feb 11, 2021

BarisSari commented Feb 11, 2021

IanMayo left a comment • edited Loading

Choose a reason for hiding this comment

robintw left a comment

Choose a reason for hiding this comment

BarisSari commented Feb 12, 2021

IanMayo commented Feb 12, 2021

BarisSari commented Feb 16, 2021

robintw left a comment

Choose a reason for hiding this comment

robintw Feb 17, 2021

Choose a reason for hiding this comment

BarisSari commented Feb 17, 2021

robintw commented Feb 17, 2021

robintw left a comment

Choose a reason for hiding this comment

BarisSari commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

IanMayo left a comment •

edited

Loading