Feature/sckan-394-395-396 - Population set - Backend, UI, and Admin UI #420

D-GopalKrishna · 2025-02-05T13:16:14Z

Task description:

https://metacell.atlassian.net/browse/SCKAN-394
https://metacell.atlassian.net/browse/SCKAN-395
https://metacell.atlassian.net/browse/SCKAN-396 - (not the third part) - that is taken care in a separate PR.

More on the PR:

The Backend changes:

Add populationset - models, URLs, views, and serializers
add another field to KS - has_statement_been_exported
along with the migrations; a custom migration - migrate_has_statement_been_exported - to fill the field for everything that is exported currently
Two constraints:
- Do not allow to go to EXPORTED if doesn't have populationset - by adding condition

conditions=[
            ConnectivityStatementStateService.is_valid,
            ConnectivityStatementStateService.has_populationset,
        ],

- Do not allow to change the populationset for a KS - if it had been ever in exported state

    def validate_population_change(self):
        if self.pk and self.has_statement_been_exported:
            original = ConnectivityStatement.objects.get(pk=self.pk)
            if original.population != self.population:
                raise ValidationError(
                    "Cannot change population set after the statement has been exported."
                )

The Composer UI changes:

update the genapi.sh
Add the copiedUISchema.population_id for - populationset
- make it read only if isDisabled: statement.has_statement_been_exported is true
make a call to populations and set it using PopulationService - and call it onLogin.

The Admin UI changes:

Add PopulationSet to the admin UI.

…odels

…xported read only

…pdating new field - has_statement_been_exported

… doesn't have EXPORT available transition

…opulationSet

…opulationService

D-GopalKrishna · 2025-02-05T13:20:40Z

backend/composer/models.py

+
+    def clean(self):
+        if not is_valid_population_name(self.name):
+            raise ValidationError(


@afonsobspinto I am not sure if throwing ValidationError is the best way we can tackle this. Do you think this is okay or can you help me find what would be right?

My preference would be to use Django validators as part of the name field definition:

name = models.CharField( max_length=20, validators=[is_valid_population_name], unique=True )

…ment_been_exported migration

D-GopalKrishna · 2025-02-05T13:30:00Z

backend/composer/services/cs_ingestion/helpers/statement_helper.py

@@ -95,7 +96,10 @@ def create_or_update_connectivity_statement(
        else:
            create_invalid_note(connectivity_statement, error_message)
    else:
-        if connectivity_statement.state != CSState.EXPORTED:
+        if (


@afonsobspinto @ddelpiano - this update is for the ingestion script - I feel that would also require an update - since we do not want anything - that does not have ConnectivityStatementStateService.has_populationset to be in the exported state as per second point here - https://metacell.atlassian.net/browse/SCKAN-394

Is the assumption correct? Or should i revert this?

That seems reasonable but @ddelpiano needs to confirm this.

Implementation wise this seems wrong syntactically (because you don't seem to be calling has_populationset) and also we would need to update the script to associate the population with the connectivity statement

updated thanks! @afonsobspinto

The syntax looks correct now yes but I believe the second part is still missing.

would need to update the script to associate the population with the connectivity statement

If we are to implement this (which I believe we should) we need to understand how to get the population info from neurondm and update the function that creates / updates connectivity statements, on ingestion, to consider that new value on creation and ignore or report an error on updates with changes on such field

I discussed this point with @ddelpiano. We are waiting on the customer to reply on how can we get the populationSet from neurondm's neuron

D-GopalKrishna · 2025-02-05T13:32:49Z

backend/composer/services/export/helpers/export_batch.py

@@ -96,3 +98,7 @@ def do_transition_to_exported(export_batch: ExportBatch, user: User):
                CSState.EXPORTED, system_user, user
            )
            cs.save()
+        else:
+            export_batch.connectivity_statements.remove(connectivity_statement)


We needed to remove connectivity_statement here - since - we do not want the one - where available transition doesn't have EXPORTED - to be exported,

And export batch is what is used in the export_services.py - to create a csv file.

I think it would make more sense to do this filtering on the create_export_batch function or at least save the changes done so that the database table keeps only the set of statements that can/were exported

D-GopalKrishna · 2025-02-05T13:34:55Z

backend/composer/utils.py

+
+
+def is_valid_population_name(name):
+    return re.match(r"^[a-zA-Z][a-zA-Z0-9_]{7,19}$", name) is not None


this is as per the requirements here - https://metacell.atlassian.net/browse/SCKAN-394 - 3rd point. But I have also included - an underscore - to accommodate the fact - we also allow underscore - but not at the start.

afonsobspinto · 2025-02-05T23:28:44Z

backend/composer/models.py

    def save(self, *args, **kwargs):
        if not self.pk and self.sentence and not self.owner:
            self.owner = self.sentence.owner

+        self.validate_population_change()


afonsobspinto · 2025-02-05T23:28:55Z

backend/composer/models.py

@@ -567,6 +600,14 @@ class ConnectivityStatement(models.Model):
    journey_path = models.JSONField(null=True, blank=True)
    statement_prefix = models.TextField(null=True, blank=True)
    statement_suffix = models.TextField(null=True, blank=True)
+    population = models.ForeignKey(


afonsobspinto · 2025-02-05T23:29:10Z

backend/composer/models.py

@@ -196,6 +204,31 @@ class Meta:
        verbose_name_plural = "Sex"


+class PopulationSet(models.Model):


afonsobspinto · 2025-02-05T23:31:10Z

backend/composer/services/state_services.py

@@ -260,6 +260,10 @@ def is_forward_connection_valid(connectivity_statement):
                return True
        return False

+    @staticmethod
+    def has_populationset(connectivity_statement) -> bool:
+        return connectivity_statement.population is not None


afonsobspinto · 2025-02-05T23:37:32Z

backend/composer/migrations/0075_migrate_has_statement_been_exported.py

+    ]
+
+    operations = [
+        migrations.RunPython(migrate_has_statement_been_exported),


afonsobspinto · 2025-02-05T23:44:33Z

backend/composer/models.py

+        conditions=[
+            ConnectivityStatementStateService.is_valid,
+            ConnectivityStatementStateService.has_populationset,
+        ],
        permission=ConnectivityStatementStateService.has_permission_to_transition_to_exported,
    )
    def exported(self, *args, **kwargs):


I think the transition effect should be to set the has_statement_been_exported to True:

self.has_statement_been_exported = True

afonsobspinto · 2025-02-05T23:54:07Z

backend/composer/services/cs_ingestion/helpers/statement_helper.py

@@ -95,7 +96,10 @@ def create_or_update_connectivity_statement(
        else:
            create_invalid_note(connectivity_statement, error_message)
    else:
-        if connectivity_statement.state != CSState.EXPORTED:
+        if (


The syntax looks correct now yes but I believe the second part is still missing.

would need to update the script to associate the population with the connectivity statement

If we are to implement this (which I believe we should) we need to understand how to get the population info from neurondm and update the function that creates / updates connectivity statements, on ingestion, to consider that new value on creation and ignore or report an error on updates with changes on such field

afonsobspinto · 2025-02-05T23:58:13Z

backend/composer/services/export/helpers/export_batch.py

@@ -96,3 +98,7 @@ def do_transition_to_exported(export_batch: ExportBatch, user: User):
                CSState.EXPORTED, system_user, user
            )
            cs.save()
+        else:
+            export_batch.connectivity_statements.remove(connectivity_statement)


I think it would make more sense to do this filtering on the create_export_batch function or at least save the changes done so that the database table keeps only the set of statements that can/were exported

afonsobspinto · 2025-02-06T00:07:31Z

frontend/src/index.tsx

-                  <App />
-                </Provider>
-              );
+          populations.setPopulations().then(() => {


Not related with this PR, but I don't love this nested callbacks. I think most of these fetches are independent, if that's the case we can use Promise.all instead to have them happening in parallel. It can be a tecnical debt card for later, cc @ddelpiano

D-GopalKrishna added 11 commits February 5, 2025 18:31

SCKAN-394 - add populationset and has_statement_been_exported to KS m…

3e91074

…odels

SCKAN-372 - validate populationset change cannot happen for exported KS

bf5b0d1

SCKAN-394 - add population set to admin and make has_statement_been_e…

9d708c5

…xported read only

SCKAN-372 - serializers, urls, and viewsets for populationset

64648cd

SCKAN-372 - migration files for new fields and custom migration for u…

6842f8a

…pdating new field - has_statement_been_exported

SCKAN-394 - test the populationset name validation

9803ce3

SCKAN-372 - utils and state services update

b7e6745

SCKAN-372 - remove the connectivity statement from export-batch if it…

d8c1c36

… doesn't have EXPORT available transition

Merge remote-tracking branch 'origin/develop' into feature/SCKAN-394

e023325

SCKAN-395 - Add PopulationService integration and API endpoints for P…

5ba1a85

…opulationSet

SCAKN_395 - Add population selection to StatementForm and integrate P…

803f5ba

…opulationService

D-GopalKrishna commented Feb 5, 2025

View reviewed changes

SCKAN-394 - Rename migration function to improve clarity in has_state…

965104d

…ment_been_exported migration

D-GopalKrishna commented Feb 5, 2025

View reviewed changes

SCKAN-394 - fix has_populationset call in statement helper

72fdc7d

afonsobspinto requested changes Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/sckan-394-395-396 - Population set - Backend, UI, and Admin UI #420

Feature/sckan-394-395-396 - Population set - Backend, UI, and Admin UI #420

D-GopalKrishna commented Feb 5, 2025 •

edited

Loading

D-GopalKrishna Feb 5, 2025

afonsobspinto Feb 5, 2025 •

edited

Loading

D-GopalKrishna Feb 5, 2025 •

edited

Loading

afonsobspinto Feb 5, 2025

D-GopalKrishna Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 6, 2025

D-GopalKrishna Feb 5, 2025

afonsobspinto Feb 5, 2025

D-GopalKrishna Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 5, 2025

afonsobspinto Feb 6, 2025



		def is_valid_population_name(name):
		return re.match(r"^[a-zA-Z][a-zA-Z0-9_]{7,19}$", name) is not None

		@@ -196,6 +204,31 @@ class Meta:
		verbose_name_plural = "Sex"


		class PopulationSet(models.Model):

Feature/sckan-394-395-396 - Population set - Backend, UI, and Admin UI #420

Are you sure you want to change the base?

Feature/sckan-394-395-396 - Population set - Backend, UI, and Admin UI #420

Conversation

D-GopalKrishna commented Feb 5, 2025 • edited Loading

More on the PR:

The Backend changes:

The Composer UI changes:

The Admin UI changes:

Choose a reason for hiding this comment

afonsobspinto Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

D-GopalKrishna Feb 5, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-GopalKrishna commented Feb 5, 2025 •

edited

Loading

afonsobspinto Feb 5, 2025 •

edited

Loading

D-GopalKrishna Feb 5, 2025 •

edited

Loading