Feature/adding mset replacement functionality 20210615 #445

cschu · 2021-06-15T08:44:31Z

No description provided.

…lity_20210615

lkuchenb

Thanks!

I don't think this would work yet, replaced_msets is joining MSetReplacementEvent with MetadataSet based on metadataset_id which is referring to the new metadataset rather than the old metadataset_s_.

The MetaDataSet class will need an attribute

MetaDataSet.replaced_by_event_id # foreign key
MetaDataSet.replaced_by_event # relationship

which refers to the replacement event if it was replaced. To be able to access also the replacement events in which a metadataset is in the "new" role there should also be a relationship attribute

MetaDataSet.replaces_event # relationship

based on MsetReplacementEvent.metadataset_id. Maybe that attribute should also be renamed to new_metadataset_id to make immediately clear which role the foreign entity linked here has.

lkuchenb · 2021-06-16T07:11:40Z

datameta/models/db.py

+    user_id          = Column(Integer, ForeignKey('users.id'), nullable=False)
+    datetime         = Column(DateTime, nullable=False)
+    # former 'is_deprecated' label from MetaDataSet
+    reason           = Column(String(140), nullable=False)


Suggested change

reason = Column(String(140), nullable=False)

label = Column(String(140), nullable=False)

…lity_20210615

datameta/api/metadatasets.py

lkuchenb · 2021-06-25T08:29:09Z

datameta/api/metadatasets.py

+    if replaces and not replaces_label:
+        return HTTPBadRequest()


Please return a proper validation error with a message. Also the inverse case should be checked and fail with a proper message (I mean the case label specified without any replacements to make)

lkuchenb · 2021-06-25T08:36:22Z

datameta/api/metadatasets.py

+            if target_mset is None:
+                raise HTTPForbidden()


The invalid replacement IDs should be collected and returned as a validation error

…lity_20210615

lkuchenb

Thanks! Generally looking good, comments below.

lkuchenb · 2021-06-29T13:37:34Z

datameta/api/metadatasets.py

+    if replaces and not replaces_label:
+        raise errors.get_validation_error(messages=['No reason (label) given for Metadataset replacement.'])  # maybe label should be reason.
+    if not replaces and replaces_label:
+        raise errors.get_validation_error(messages=["No metadataset ids specified (replacement reason (label) is given."])


Suggested change

raise errors.get_validation_error(messages=["No metadataset ids specified (replacement reason (label) is given."])

raise errors.get_validation_error(messages=["No metadataset IDs specified (replacement reason (label) is given."])

lkuchenb · 2021-06-29T14:39:52Z

datameta/api/metadatasets.py

@@ -279,7 +311,7 @@ def get_metadatasets(request: Request) -> List[MetaDataSetResponse]:
        query = query.filter(Submission.date < submitted_before.astimezone(timezone.utc))
    if awaiting_service is not None:
        if awaiting_service not in readable_services_by_id:
-            raise errors.get_validation_error(messages=['Invalid service ID specified'], fields=['awaitingServices'])
+            raise errors.get_validation_error(messages=['Invalid service ID specified'], fields=['awaitingService'])


lkuchenb · 2021-06-30T11:44:14Z

datameta/api/metadatasets.py

+        for _, target_mset in msets:
+            target_mset.replaced_via_event_id = mset_repl_evt.id


This requires checking whether replaced_via_event_id was previously set. Only if it is None, a replacement is legal. Otherwise, a validation error should be returned.

lkuchenb · 2021-06-30T13:24:55Z

datameta/api/metadatasets.py

        if missing_msets:
            messages, entities = zip(*missing_msets)
            raise errors.get_validation_error(messages=messages, entities=entities)

+        already_replaced = [
+            (f"MetaDataSet was already replaced via event {target_mset.replaced_via_event_id}", mset_id)


This exposes an internal database ID. Also, I'd hide the event logic to the user and rather print the ID of the new mset (get_identifier).

lkuchenb · 2021-07-07T08:28:19Z

datameta/api/metadatasets.py

@@ -220,7 +220,7 @@ def replace_metadatasets(request: Request) -> SubmissionResponse:

    if missing_msets:
        messages, entities = zip(*missing_msets)
-        raise errors.get_validation_error(messages=list(messages), entities=list(entities))
+        raise errors.get_validation_error(messages=list(messages), entities=list(entities))  # @lkuchenb: any idea why these list-casts are necessary to please mypy? they're not required in e.g validation.py:63


zip() returns tuples, not lists

that i know :P but why does it not complain in validation.py, line 63, which also uses zip-derived values for the arguments?

No idea. But we should probably not be so strict about the argument type in errors.get_validation_error and relax it to Iterable

Suggested change

raise errors.get_validation_error(messages=list(messages), entities=list(entities)) # @lkuchenb: any idea why these list-casts are necessary to please mypy? they're not required in e.g validation.py:63

raise errors.get_validation_error(messages=list(messages), entities=list(entities))

cschu · 2021-07-07T10:08:36Z

The breaking test fixed via 0289f2a is probably due to something I don't understand regarding the fixtures.

… to submission)

lkuchenb

Thanks a lot, also for already incorporating the visualization of the replacements in the "View" pane!

Here are my remarks:

datameta/api/openapi.yaml

datameta/api/users.py

datameta/models/db.py

lkuchenb · 2021-07-08T16:25:30Z

datameta/api/metadatasets.py

@@ -220,7 +220,7 @@ def replace_metadatasets(request: Request) -> SubmissionResponse:

    if missing_msets:
        messages, entities = zip(*missing_msets)
-        raise errors.get_validation_error(messages=list(messages), entities=list(entities))
+        raise errors.get_validation_error(messages=list(messages), entities=list(entities))  # @lkuchenb: any idea why these list-casts are necessary to please mypy? they're not required in e.g validation.py:63


Suggested change

raise errors.get_validation_error(messages=list(messages), entities=list(entities)) # @lkuchenb: any idea why these list-casts are necessary to please mypy? they're not required in e.g validation.py:63

raise errors.get_validation_error(messages=list(messages), entities=list(entities))

tests/integration/test_bulkdelete.py

datameta/api/metadatasets.py

datameta/api/openapi.yaml

lkuchenb · 2021-08-03T19:24:30Z

datameta/api/ui/view.py

@@ -224,6 +224,7 @@ def post(request: Request):
                submission_datetime   = mdata_set.submission.date.isoformat(),
                submission_label      = mdata_set.submission.label,
                service_executions    = service_executions,
+                replaced_by           = get_identifier(mdata_set.replaced_via_event.new_metadataset) if mdata_set.replaced_via_event else None


This will fire two additional queries for every mdata_set, first when accessing the replaced_via_event attribute, then when accessing the new_metadataset attribute. Since we know we will be accessing those fields, we can instruct SQLAlchemy not to use lazy loading, but to join the corresponding relations already when we query the mdata_sets. Please add the corresponding joins in L177.

datameta/static/js/view.js

Co-authored-by: Leon Kuchenbecker <[email protected]>

…eted

…lity_20210615

ckaipf · 2023-02-03T08:17:22Z

I tried to merge it yesterday, only some minor conflicts arose.

cschu added 2 commits June 15, 2021 08:16

added MsetReplacementEvent model

b73c2c9

fixed alembic complaint

c61ae1d

cschu added the Do Not Merge label Jun 15, 2021

cschu requested a review from lkuchenb June 15, 2021 08:44

cschu added 3 commits June 15, 2021 09:11

added alembic script for revision related to b73c2c9

723ec8b

pleasing flake8

e53527b

Merge branch 'develop' into feature/adding_mset_replacement_functiona…

157ef00

…lity_20210615

lkuchenb requested changes Jun 16, 2021

View reviewed changes

cschu added 8 commits June 16, 2021 19:40

Merge branch 'develop' into feature/adding_mset_replacement_functiona…

0708ccb

…lity_20210615

fixed MsetReplacement/MetaDataSet key issues (with @lkuchenb)

3dbc0f4

merged upstream

df35eba

pleasing flake8

1b3a3fc

Merge branch 'develop' into feature/adding_mset_replacement_functiona…

9f09d96

…lity_20210615

added initial mset replacement logic

729ddcc

merged upstream

85ade91

pleasing flake8 and mypy

2bd9b75

lkuchenb reviewed Jun 25, 2021

View reviewed changes

cschu added 9 commits June 25, 2021 15:03

addressing @lkuchenb's code review

523208f

added user.can_update flag

9068be3

generated alembic script pertaining to 9068be3

2392560

pleasing flake8

54d3b63

pleasing flake8

1c9a668

Merge branch 'develop' into feature/adding_mset_replacement_functiona…

1ffefdb

…lity_20210615

fixing the fixtures

bf20b41

fixing the fixtures

67fe823

addressing mypy error

9accbed

lkuchenb requested changes Jun 30, 2021

View reviewed changes

implemented code review requests (@lkuchenb)

5db3d83

lkuchenb requested changes Jun 30, 2021

View reviewed changes

patched exposed event id

b92e67e

cschu added 4 commits July 6, 2021 16:47

works

ac112b7

weird.

4a35bce

weird.

e0803da

weird

6f75123

lkuchenb reviewed Jul 7, 2021

View reviewed changes

cschu added 3 commits July 7, 2021 09:07

refactored + working

d0b3f6b

added preliminary test for metadataset replacement

e77bf56

workaround to re-enable bulk-deletion test

0289f2a

cschu added 10 commits July 14, 2021 07:11

removed redundant label from msetreplacementevents (label is attached…

ac2b431

… to submission)

cleaning

3e1bfcd

added can_update=True to initial user

c6f22a0

metadatasetresponses now include replaced/replaces information (#462)

9e88373

added more testcases

d882ffe

pleasing flake8

d395384

pleasing mypy

b7cf1ed

pleasing mypy

e47a56d

removed comment

b87e481

added information about replaced metadatasets to datatable

455ee05

lkuchenb requested changes Aug 3, 2021

View reviewed changes

cschu and others added 9 commits August 3, 2021 22:33

Apply suggestions from @lkuchenb's code review

27163f5

Co-authored-by: Leon Kuchenbecker <[email protected]>

implemented suggestions from @lkuchenb's code review

306cf35

merge upstream (github code review)

d353255

dealt with fallout from renaming msetreplacements-table

455626a

fixed bulk deletion test

00738a1

fixed failing stage_and_submit test

ea30854

pleasing mypy

cbe39eb

fixed issue that prevented new user registration requests to be compl…

dd036cc

…eted

Merge branch 'develop' into feature/adding_mset_replacement_functiona…

216a08b

…lity_20210615

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/adding mset replacement functionality 20210615 #445

Feature/adding mset replacement functionality 20210615 #445

cschu commented Jun 15, 2021

lkuchenb left a comment

lkuchenb Jun 16, 2021

lkuchenb Jun 25, 2021

lkuchenb Jun 25, 2021

lkuchenb left a comment

lkuchenb Jun 29, 2021

lkuchenb Jun 29, 2021

lkuchenb Jun 30, 2021

lkuchenb Jun 30, 2021

lkuchenb Jul 7, 2021

cschu Jul 7, 2021

lkuchenb Jul 7, 2021 •

edited

Loading

lkuchenb Jul 8, 2021

cschu commented Jul 7, 2021

lkuchenb left a comment

lkuchenb Jul 8, 2021

lkuchenb Aug 3, 2021 •

edited

Loading

ckaipf commented Feb 3, 2023

	reason = Column(String(140), nullable=False)
	label = Column(String(140), nullable=False)

	raise errors.get_validation_error(messages=["No metadataset ids specified (replacement reason (label) is given."])
	raise errors.get_validation_error(messages=["No metadataset IDs specified (replacement reason (label) is given."])

		for _, target_mset in msets:
		target_mset.replaced_via_event_id = mset_repl_evt.id

Feature/adding mset replacement functionality 20210615 #445

Are you sure you want to change the base?

Feature/adding mset replacement functionality 20210615 #445

Conversation

cschu commented Jun 15, 2021

lkuchenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkuchenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkuchenb Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschu commented Jul 7, 2021

lkuchenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkuchenb Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

ckaipf commented Feb 3, 2023

lkuchenb Jul 7, 2021 •

edited

Loading

lkuchenb Aug 3, 2021 •

edited

Loading