feature/#1197 pass job as metadata for event #1277
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
There was a problem hiding this comment.
- We are missing the unit tests covering the new code on both reload.py and submission.py modules.
- Even if the code looks smart, I don't like giving the notification responsibility to the entity itself. For me, it should be the repository/self_setter layer.
We can discuss and try to challenge your solution to see if we can propose a better design.
jrobinAV
added
Core
toan-quach
force-pushed
the
feature/#1197-add-job-to-on-submission-change-event
branch
from
44e9311
to
a47bac4
Compare
| EventOperation.UPDATE, | ||
| "submission_status", | ||
| submission._submission_status, | ||
| job_triggered_submission_status_changed=job.id, |
There was a problem hiding this comment.
This parameter name is exposed to the user through the event. It becomes an attribute of the event. We must find a better name. What about: "job_id"?
There was a problem hiding this comment.
a bit vague imho, how about trigger_job_id?
| @@ -174,7 +174,10 @@ def submission_status_callback(self, submission_id: t.Optional[str]): | |||
| self.gui._call_user_callback( | |||
| client_id, | |||
| submission_name, | |||
| [core_get(submission.entity_id), {"submission_status": new_status.name}], | |||
| [ | |||
| core_get(submission.entity_id), | |||
There was a problem hiding this comment.
core_get(submission.entity_id) is a scenario. I guess we want to pass the submission instead: core_get(submission.id).
@FredLL-Avaiga @toan-quach What do you think?
There was a problem hiding this comment.
the doc indicates : 1st param is a submittable (scenario or sequence)
We could add the submission to the detail but I think the main object is rightly the submittable.
There was a problem hiding this comment.
well, I'm not sure either so I don't have any comments on it
#1197