-
Notifications
You must be signed in to change notification settings - Fork 0
Unify activity status #513
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| Enum(ValidationStatus, name="me_model_validation_status"), | ||
| default=ValidationStatus.created, | ||
| ) | ||
| validation_status: Mapped[ActivityStatus] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation_status is an attribute of me_model, and not the status of an activity, so it seems strange to define it on the entity and use the same enum.
Where is validation_status used and how is it updated?
Should it be replaced with ValidationResult?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's the status of the validation process, it seems to me it should be an activity status. Not sure this is the case however. @jdcourcol wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to ask @pgetta . I don't see a place in the code where that flag is being updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, there seems to be a certain disconnect with respect to this property.
In the UI it's used in the ME-model list view as a separate column with binary value (Validated: true/false) as well as in the detailed and mini detailed view.
In Bluenaas, when the ME-model is getting created - the property is set to created but the validation process doesn't update that.
From what I see only a subset of public models have this property set to done.
At the moment we have a single validation job, but it has a set of different tasks each producing and registering it's own ValidationResult entity.
If the idea for the validation_status property is to track the validation job - I'll have to update the logic in Bluenaas. @jdcourcol could you please confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this validated property should come from the ValidationResult entity. and we should get rid of the validation_status property.
07dc599 to
4b10d99
Compare
chr-pok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @eleftherioszisis, nice work!
Activity status moved to activity table and unified with ActivityStatus:
Issue: #511
Activities the status of which moved to activity parent table:
Columns that remapped to ActivityStatus
Activities without prior status column set to "done" because activity parent table has a status now
Given that this is a breaking change, fyi @james-isbister @chr-pok @AurelienJaquier @ilkilic