Enum refactoring #978

SofiaSazonova · 2024-01-16T10:44:44Z

Feature or Bugfix

Refactoring

Detail

Get rid of redefinitions of the same Enums. Used class GraphQLEnumMapper where it's required
Unhardcoded enum-constants for migrations and tests

Security N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

backend/__init__.py

backend/dataall/modules/dataset_sharing/db/share_object_models.py

dbalintx · 2024-01-17T15:46:59Z

backend/dataall/modules/datasets/api/dataset/resolvers.py

@@ -52,15 +52,15 @@ def resolve_user_role(context: Context, source: Dataset, **kwargs):
                .first()
            )
            if share and (
-                share.owner == context.username or share.principalId in context.groups
+                    share.owner == context.username or share.principalId in context.groups


not sure if these formatting changes are needed in this file

dbalintx · 2024-01-17T15:49:55Z

backend/dataall/modules/datasets/api/table/enums.py

-    updated = 'updated'
-    label = 'label'
-
-
 class DatasetTablePreviewStatus(GraphQLEnumMapper):


maybe move this enum to datasets_base/constants/enums.py and get rid of this file?

This enum is only used in dataall/modules/datasets/api/table/types.py:100 for the class DatasetTablePreviewResult that as far as I see it is not used. Maybe we can delete both the Enum and the gqlObjectType

backend/dataall/modules/dataset_sharing/common/enums.py

dbalintx · 2024-01-18T08:58:44Z

backend/migrations/versions/5e5c84138af7_backfill_confidentiality.py

@@ -74,8 +76,8 @@ def upgrade():
        print('Updating datasets...')
        datasets: [Dataset] = session.query(Dataset).all()
        for dataset in datasets:
-            if dataset.confidentiality not in ['Unclassified', 'Official', 'Secret']:
-                dataset.confidentiality = 'Unclassified'
+            if dataset.confidentiality not in ConfidentialityClassification:


are you sure this is working?
you could do something like:

if not any(value == dataset.confidentiality for value in ConfidentialityClassification.__dict__.values()):

I made a mistake

Check if enum contains value is now python 3.10 compatible

Check if enum contains value is now python 3.10 compatible Remove unused enum and object DatasetTablePreview rename enum folder to 'common/enums.py'

noah-paige · 2024-01-22T19:52:33Z

Hi @SofiaSazonova @dlpzx - what is left to be done before we can push forward testing and approving this PR...

I see we have some common/enums.py files still in the repo for dataset_sharing and datasets_base- did we decide best to move underapi/` folder or do we want to keep it as is now?

I am open for alternative approaches / further discussion - want to align so I can help review and push this PR through

SofiaSazonova · 2024-01-24T12:02:16Z

Hi @SofiaSazonova @dlpzx - what is left to be done before we can push forward testing and approving this PR...

I see we have some common/enums.py files still in the repo for dataset_sharing and datasets_base- did we decide best to move underapi/` folder or do we want to keep it as is now?

I am open for alternative approaches / further discussion - want to align so I can help review and push this PR through

@noah-paige I moved all enums to "common" folder since they are used not only in api, but also in models and etc. If you or @dlpzx insist I can move them back to api folder, but in my opinion it's odd))

dlpzx · 2024-01-24T12:56:06Z

tests/modules/datasets/conftest.py

+            name: str,
+            owner: str,
+            group: str,
+            confidentiality: str = None


remove additional indentation

dlpzx · 2024-01-24T12:56:20Z

tests/modules/datasets/conftest.py

-        label: str
+            organization: Organization,
+            environment: Environment,
+            label: str


Remove additional indentation

dlpzx · 2024-01-24T13:12:22Z

Hi @SofiaSazonova @dlpzx - what is left to be done before we can push forward testing and approving this PR...
I see we have some common/enums.py files still in the repo for dataset_sharing and datasets_base- did we decide best to move underapi/` folder or do we want to keep it as is now?
I am open for alternative approaches / further discussion - want to align so I can help review and push this PR through

@noah-paige I moved all enums to "common" folder since they are used not only in api, but also in models and etc. If you or @dlpzx insist I can move them back to api folder, but in my opinion it's odd))

In my opinion there is no need to create a new layer common in each module. db, api, services already import each other, it is not like they are independent sub-modules. I pictured layers as functional components with clearly defined purposes, common is a bit too generic and has the risk of being a "here we can store anything" layer. That's why I would add the constants to an existing layer.

Now, which one should it be? We can argue that since it is business logic they should be defined as part of services in a file such as ml_studio_enums.py. Another option would be to add it into api because the enums inherit the class GraphQLEnumMapper. I am 50% 50% with both options.

Another idea that I got is to rename the enums, instead of enum.py name them as modulename_enums.py to make it easier to search and manage each module separately. Similar to <modulename_models.py in db or mlstudio_service.py in services. What do you think?

SofiaSazonova · 2024-01-24T13:40:57Z

I agree, that naming modulename_enums.py is much better.
As for storage of the file...I think may be then models is better? Like it's a part of field definition as well.

SofiaSazonova · 2024-01-24T13:57:10Z

Also, there is no api folder in datasets_base. So, for unification I tend more to services, as you proposed

Naming modulename_enums.py

SofiaSazonova · 2024-01-24T16:54:18Z

So, I moved enums to services and renamed them as module_enums

dlpzx · 2024-01-26T07:58:04Z

Overall changes look good @SofiaSazonova. I am doing a last test in AWS before approving

CICD completes
Share Enum - share functionality works as expected
Dataset Enums - creation of Dataset
Organization Enums - create Organization

dlpzx

Tested in AWS

Sofia Sazonova added 2 commits January 15, 2024 17:20

Enum refactoring. Remove redefinition

a51644d

Enum refactoring. Unhardcode constants

84d8c4b

dlpzx reviewed Jan 16, 2024

View reviewed changes

backend/__init__.py Outdated Show resolved Hide resolved

Delete backend/__init__.py

b096a31

dbalintx reviewed Jan 17, 2024

View reviewed changes

backend/dataall/modules/dataset_sharing/db/share_object_models.py Outdated Show resolved Hide resolved

dbalintx reviewed Jan 17, 2024

View reviewed changes

dlpzx reviewed Jan 17, 2024

View reviewed changes

backend/dataall/modules/dataset_sharing/common/enums.py Outdated Show resolved Hide resolved

dlpzx reviewed Jan 17, 2024

View reviewed changes

backend/dataall/modules/dataset_sharing/common/enums.py Outdated Show resolved Hide resolved

dlpzx previously approved these changes Jan 17, 2024

View reviewed changes

dbalintx reviewed Jan 18, 2024

View reviewed changes

Sofia Sazonova added 2 commits January 18, 2024 16:56

Remove relative imports

4ba70da

Check if enum contains value is now python 3.10 compatible

Remove relative imports

61fbf40

Check if enum contains value is now python 3.10 compatible Remove unused enum and object DatasetTablePreview rename enum folder to 'common/enums.py'

dlpzx reviewed Jan 24, 2024

View reviewed changes

Sofia Sazonova added 3 commits January 24, 2024 16:28

Move enums from common to services

2a693f4

Naming modulename_enums.py

Merge branch 'mda-main' into enum-refactoring

bb4a9d0

Organization_enum refactoring

8c4a488

Organization_enums moved to services

f6a15ae

dlpzx approved these changes Jan 26, 2024

View reviewed changes

dlpzx merged commit ffb5949 into data-dot-all:main Jan 26, 2024
8 checks passed

dlpzx mentioned this pull request Aug 2, 2024

Row/Column Level Data Filters #1438

Merged

noah-paige mentioned this pull request Aug 5, 2024

Metadata forms-2: Create, display list, search list #1444

Merged

SofiaSazonova deleted the enum-refactoring branch October 3, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum refactoring #978

Enum refactoring #978

SofiaSazonova commented Jan 16, 2024

dbalintx Jan 17, 2024

dbalintx Jan 17, 2024

dlpzx Jan 17, 2024

dbalintx Jan 18, 2024 •

edited

Loading

noah-paige commented Jan 22, 2024

SofiaSazonova commented Jan 24, 2024

dlpzx Jan 24, 2024

dlpzx Jan 24, 2024

dlpzx commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

dlpzx commented Jan 26, 2024 •

edited

Loading

dlpzx left a comment

Enum refactoring #978

Enum refactoring #978

Conversation

SofiaSazonova commented Jan 16, 2024

Feature or Bugfix

Detail

Security N/A

dbalintx Jan 17, 2024

Choose a reason for hiding this comment

dbalintx Jan 17, 2024

Choose a reason for hiding this comment

dlpzx Jan 17, 2024

Choose a reason for hiding this comment

dbalintx Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

noah-paige commented Jan 22, 2024

SofiaSazonova commented Jan 24, 2024

dlpzx Jan 24, 2024

Choose a reason for hiding this comment

dlpzx Jan 24, 2024

Choose a reason for hiding this comment

dlpzx commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

SofiaSazonova commented Jan 24, 2024

dlpzx commented Jan 26, 2024 • edited Loading

dlpzx left a comment

Choose a reason for hiding this comment

dbalintx Jan 18, 2024 •

edited

Loading

dlpzx commented Jan 26, 2024 •

edited

Loading