enhance group events DEX message for additional fields such as scope description by bpow · Pull Request #86 · clingen-data-model/gpm

bpow · 2024-11-05T15:54:47Z

Opening pull request as @tmbattey-2021 indicated this is ready for review.

tmbattey-2021 · 2024-11-05T16:17:32Z

Configs.json shouldn't be part of this change request. It's related to the roles change.

bpow · 2024-11-05T19:10:53Z

Configs.json shouldn't be part of this change request. It's related to the roles change.

I'm guessing that is probably true for groups.php as well?

Since I have needed to rebase to current main, anyway, I will amend this commit to not include configs.json and groups.php.

bpow · 2024-11-05T19:18:47Z

Modified commit to just include the files relevant to this change. Older version of the commit is in branch reconciliation_gold_with_extraneous, but that branch should be removed, @tmbattey-2021 when you can be sure you have those changes tracked elsewhere if needed.

bpow · 2024-11-05T19:30:35Z

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php

+    return [
+        'expert_panel' => array_merge([
+            'id' => $this->group->uuid,
+            'long_name' => $this->group->name,


As we discussed earlier today, it may be helpful to review the call. I thought the website team was OK with receiving things labelled as either long_name or name, as long as we are consistent, and there are many other messages where the group name is sent as name. Personally, I lean towards name since that's also what it is called in the underlying db model.

Yes, I understood from the call that other messages use name. The code is in good enough shape to send to gpm demo and revise later. There was only one example of name given on the call.

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php

bpow · 2024-11-05T19:34:44Z

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php

+        // Conditionally add vcep fields below if type is 'vcep'
+        $this->group->fullType->name === 'vcep' ? ['clinvar_id' => null] : [],
+        $this->group->fullType->name === 'vcep' ? ['clinvar_url' => null] : [],
+        $this->group->fullType->name === 'vcep' ? ['cspec_url' => $this->group->expertPanel->affiliation_id] : [],


This is just an affiliation id, not a URL, so the field should probably have a different name. I thought we had talked about the website team generating this url from the affiliation ID, right?

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php

bpow · 2024-11-05T19:40:55Z

app/Modules/Group/Events/GroupStatusUpdated.php

+
+        // Only process the event if the status is 2 or 5
+    if (!in_array($this->group->group_status_id, [2, 3, 4, 5])) {
+        return;


Does this even do anything different? It looks to just be returning from the constructor "early", but it would also return right after this statement.

app/Modules/Group/Events/GroupStatusUpdated.php

tmbattey-2021 · 2024-11-06T13:41:38Z

I added the filter by status, as the original requirement was to send active and the other. It works. I’ll remove the magic numbers. What is your suggestion for a better filter location?

…

On Tue, Nov 5, 2024 at 2:41 PM, Bradford Powell ***@***.***(mailto:On Tue, Nov 5, 2024 at 2:41 PM, Bradford Powell <<a href=)> wrote: @bpow commented on this pull request. --------------------------------------------------------------- In [app/Modules/Group/Events/GroupStatusUpdated.php](#86 (comment)): > @@ -23,6 +23,11 @@ class GroupStatusUpdated extends GroupEvent */ public function __construct(public Group $group, public GroupStatus $newStatus, public GroupStatus $oldStatus) { + + // Only process the event if the status is 2 or 5 + if (!in_array($this->group->group_status_id, [2, 3, 4, 5])) { + return; Does this even do anything different? It looks to just be returning from the constructor "early", but it would also return right after this statement. — Reply to this email directly, [view it on GitHub](#86 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ANJDPNN6YJTJA6VHX5KMDPLZ7ENNZAVCNFSM6AAAAABRG34KCKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMJWGU2DAOBVGQ). You are receiving this because you were mentioned.Message ID: ***@***.***>

bpow · 2024-11-06T20:54:57Z

I added the filter by status, as the original requirement was to send active and the other. It works. I’ll remove the magic numbers. What is your suggestion for a better filter location?

Did you test that this actually filters by status as you intended, because I don't think that placing a guard in the constructor like this (with just a return) would do so.

There are several places I've mentioned this that may be relevant:

In email earlier yesterday afternoon, where I suggested that a guard like this should probably go in something like a "shouldPublish" function
In comment last month in changes to reconcile current gpm message to website data #70 where I referenced shouldPublish as a function that's part of PublishableEvent used to determine whether notification of an event goes to the dx (in that context regarding the publishing of non-vcep, non-gcep events)
In CGSP-505 comment to make sure you had seen the above comment in PR#70

Although as mentioned in another CGSP-505 comment, it doesn't look like GroupEvent ever implements PublishableEvent, so maybe something is handled differently.

This should definitely be a topic for additional documentation when you are able to identify the process (and again, I have suggested that the entirety of which messages are published at which events should be better documented).

bpow · 2024-11-19T18:06:30Z

app/Modules/Group/Events/GroupStatusUpdated.php

-    if (!in_array($this->group->group_status_id, [2, 3, 4, 5])) {
-        return;
-    }
+        if ($this->group->group_status_id != config('groups.statuses.applying.id')) {


You need to test locally to see if having this here really prevents the message from being sent when a group is in status of applying. I do not think it does, so this is blocking merge into main.

bpow · 2024-11-19T18:07:36Z

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php

+                    'scope_description' => $this->group->expertPanel->scope_description,
+                ],
+                // Conditionally add vcep fields below if type is 'vcep'
+                $this->group->fullType->name === 'vcep' ? ['clinvar_id' => null] : [],


I am hesitant to merge to main while these vcep fields (clinvar_id and clinvar_url) remain null as placeholders.

tmbattey-2021 · 2024-11-19T18:31:52Z

I tested locally last week and said in our status meeting that it didn’t work. I’ll remove this guard.

…

On Tue, Nov 19, 2024 at 1:06 PM, Bradford Powell ***@***.***(mailto:On Tue, Nov 19, 2024 at 1:06 PM, Bradford Powell <<a href=)> wrote: @bpow commented on this pull request. --------------------------------------------------------------- In [app/Modules/Group/Events/GroupStatusUpdated.php](#86 (comment)): > public function __construct(public Group $group, public GroupStatus $newStatus, public GroupStatus $oldStatus) { - // Only process the event if the status is 2 or 5 - if (!in_array($this->group->group_status_id, [2, 3, 4, 5])) { - return; - } + if ($this->group->group_status_id != config('groups.statuses.applying.id')) { You need to test locally to see if having this here really prevents the message from being sent when a group is in status of applying. I do not think it does, so this is blocking merge into main. — Reply to this email directly, [view it on GitHub](#86 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ANJDPNOJLKGSKYGIAVKZX7D2BN43ZAVCNFSM6AAAAABRG34KCKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBWGIZTQNZXGM). You are receiving this because you were mentioned.Message ID: ***@***.***>

…description original commit was modified by bpow to remove extraneous file changes in configs.json and groups.php

This should not been needed since the GroupEvent is in the file's namespace. Furthermore, it had been added to the previous commit spuriously (not related to the other parts of the commit. This should probably be a "fixup" to prior commit before eventual merge to main, but keeping in history for now for tracking purposes

changing this to 2.0.0 should be one of the last things we do before moving to production as protection against the possibility of accidentally publishing messages with a different format under the eventual 2.0.0 version number

* not needed for status since group_status is specified as NOT NULL and foreign key, so should always be present. I'd rather see an error than have it silently produce an empty value * use null-safe dereference rather than optional for parent_group. This is considered by many to be easier to read, and as it was written (with "->name" outside of the parentheses, I don't think it would have worked for all of the null-possible cases as it was written, anyway (if there was no parent group, then `optional($this->group->parentGroup)` would give an optional with null value, so we'd get an error when trying to access ->name from that

bpow · 2024-12-17T16:43:06Z

Replacing with #96

bpow force-pushed the reconciliation_gold branch from bf608b2 to aa3dbff Compare November 5, 2024 19:06

bpow force-pushed the reconciliation_gold branch from aa3dbff to d5846e8 Compare November 5, 2024 19:17

bpow commented Nov 5, 2024

View reviewed changes

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php Outdated Show resolved Hide resolved

bpow commented Nov 5, 2024

View reviewed changes

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php Outdated Show resolved Hide resolved

bpow commented Nov 5, 2024

View reviewed changes

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php Outdated Show resolved Hide resolved

bpow commented Nov 5, 2024

View reviewed changes

app/Modules/Group/Events/Traits/IsPublishableApplicationEvent.php Outdated Show resolved Hide resolved

bpow commented Nov 5, 2024

View reviewed changes

app/Modules/Group/Events/GroupStatusUpdated.php Outdated Show resolved Hide resolved

bpow commented Nov 19, 2024

View reviewed changes

bpow force-pushed the reconciliation_gold branch from 6641429 to f465254 Compare November 20, 2024 15:34

tmbattey-2021 and others added 9 commits December 17, 2024 11:20

enhance group events DEX message for additional fields such as scope …

716f26f

…description original commit was modified by bpow to remove extraneous file changes in configs.json and groups.php

structure of message is set

a6fc14c

removed magic numbers

9e6b343

going back to original file for group status updates

f5ba1cf

removed null fields for vcep groups

56445d7

backing off version of dx messages for now

cf001a5

changing this to 2.0.0 should be one of the last things we do before moving to production as protection against the possibility of accidentally publishing messages with a different format under the eventual 2.0.0 version number

spacing cleanup

b38def0

bpow force-pushed the reconciliation_gold branch from a270f5d to 95e765b Compare December 17, 2024 16:26

bpow mentioned this pull request Dec 17, 2024

changes to reconcile current gpm message to website data #70

Closed

bpow mentioned this pull request Dec 17, 2024

Website integration #96

Draft

bpow closed this Dec 17, 2024

Conversation

bpow commented Nov 5, 2024

Uh oh!

tmbattey-2021 commented Nov 5, 2024

Uh oh!

bpow commented Nov 5, 2024

Uh oh!

bpow commented Nov 5, 2024

Uh oh!

bpow Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tmbattey-2021 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bpow Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bpow Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmbattey-2021 commented Nov 6, 2024 via email

Uh oh!

bpow commented Nov 6, 2024

Uh oh!

bpow Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

bpow Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

tmbattey-2021 commented Nov 19, 2024 via email

Uh oh!

bpow commented Dec 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants