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

Feature/sckan-273 #354

Merged
merged 5 commits into from
Nov 12, 2024
Merged

Feature/sckan-273 #354

merged 5 commits into from
Nov 12, 2024

Conversation

D-GopalKrishna
Copy link
Contributor

@D-GopalKrishna D-GopalKrishna commented Nov 7, 2024

Jira ticket - SCKAN-273

About the issue - of statements getting converted from exported to invalid and vice versa.

Previously:
ConnectivityStatement.objects.filter(reference_uri=reference_uri).update(**defaults)

IMO - we should not update the default state here (since the default state is EXPORTED). So the problem we were facing - happened because if the statement had any changes, then - it was getting converted to EXPORTED.


This alone should fix the issue. Since now - it is not getting converted to EXPORTED, and hence would not be needing the conversion back to INVALID. However following is a good to have.

i.e.
connectivity_statement = ConnectivityStatement.objects.filter(reference_uri=reference_uri).first()

instead of

fields_to_refresh = [field for field in defaults.keys() if field != 'state']
connectivity_statement.refresh_from_db(fields=fields_to_refresh)

since it doesn't get the latest for the state field. Which later caused errors (before the above fix) here.

Example of bad update before:
Once the statement was converted to EXPORTED state due to defaults, line still thinks it is in invalid state (and hence doesn't move it to INVALID - but will convert this very statement to INVALID the next time we run the script). However, we did update and fetched the latest the before.

fields_to_refresh = [field for field in defaults.keys() if field != 'state']
connectivity_statement.refresh_from_db(fields=fields_to_refresh)
defaults_without_state = {field: value for field, value in defaults.items() if field != 'state'}
ConnectivityStatement.objects.filter(reference_uri=reference_uri).update(**defaults_without_state)
Copy link
Contributor Author

@D-GopalKrishna D-GopalKrishna Nov 8, 2024

Choose a reason for hiding this comment

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

@afonsobspinto - keeping the filter instead of the get here - since we cannot update a get (can only update a queryset)
AttributeError: 'ConnectivityStatement' object has no attribute 'update'

also doing connectivity_statement = ConnectivityStatement.objects.filter(reference_uri=reference_uri).first() since, we get

ConnectivityStatement.objects.filter(reference_uri=reference_uri).update(**defaults_without_state)
>>> 1

as return value after the update.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we could do:

        ```
        ConnectivityStatement.objects.filter(reference_uri=reference_uri).update(**defaults_without_state)
        connectivity_statement.refresh_from_db()
        ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the following?

fields_to_refresh = [field for field in defaults.keys() if field != 'state']
connectivity_statement.refresh_from_db(fields=fields_to_refresh)

since the connectivity_statement.refresh_from_db() would lead to AttributeError: Direct state modification is not allowed.

@afonsobspinto
Copy link
Member

afonsobspinto commented Nov 11, 2024

Just to clarify a little bit the original problem.

As you mentioned, if an existent statement had changes it would transition to exported. This is in theory non problematic because later there's a set of instructions responsible to move it to invalid. However the state of the statement was not refreshed and that's where the real problem lies.

Here's an example of the problem:

Statement A exists and is Invalid
Statement A moves to Exported by default due to being part of the ingestion
We check if statements with errors are in Invalid state, statement A is still with state set as Invalid so no transition is applied.
We end up with Statement A in Exported even tho it should be in Invalid

Copy link
Member

@afonsobspinto afonsobspinto left a comment

Choose a reason for hiding this comment

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

I understand that the solution proposed is to not do the initial transition to Exported.
Let's see the possible scenarios:

  1. Statement A is in Exported and has no problems ✅
  2. Statement A is in Exported and has problems, then it's moved to Invalid by the transition check ✅
  3. Statement A is in Invalid and has problems, a new note is added, no transition is necessary ✅
  4. Statement A is in Invalid and has no problems. It should be moved to exported but currently I don't think it's. ❌

I think we need to add something, after the update without the state, on the lines of:

        if connectivity_statement.state != CSState.EXPORTED:
            # Use the FSM transition method
            connectivity_statement.exported()
            connectivity_statement.save()
            
            ```

@@ -50,6 +50,9 @@ def create_or_update_connectivity_statement(statement: Dict, sentence: Sentence,
do_transition_to_invalid_with_note(connectivity_statement, error_message)
else:
create_invalid_note(connectivity_statement, error_message)
else:
if connectivity_statement.state != CSState.EXPORTED:
do_transition_to_exported(connectivity_statement)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to EXPORTED state (default) if not in EXPORTED state - if there's no validation errors.

Reason to include this: (credit thanks - @afonsobspinto : ) )

If a statement was in INVALID state in the past; but if it has no validation errors when running it again in the future, then it should to be moved to EXPORTED state.

To cover the fourth edge case here - #354 (review)

afonsobspinto
afonsobspinto previously approved these changes Nov 12, 2024
@@ -634,7 +634,10 @@ def npo_approved(self, *args, **kwargs):

@transition(
field=state,
source=CSState.NPO_APPROVED,
source=[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: allow to export from invalid.

@D-GopalKrishna D-GopalKrishna changed the title Feature/sckan-273 - Feature/sckan-273 - Nov 12, 2024
@D-GopalKrishna D-GopalKrishna changed the title Feature/sckan-273 - Feature/sckan-273 Nov 12, 2024
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