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

improve group events #787

Merged
merged 2 commits into from
Nov 2, 2023
Merged

improve group events #787

merged 2 commits into from
Nov 2, 2023

Conversation

tpolecat
Copy link
Member

This changes the group event stream to trigger when the contents of a group change, so you get an event when an observation is created, and two events when an observation is moved.

@tpolecat
Copy link
Member Author

@hugo-vrijswijk maybe you can say a few words about how you intend to consume this event. We could add more information (like a CONTENTS_CHANGED event type for instance) but I'm not sure what you need.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (ffb5f09) 78.94% compared to head (ba53731) 79.39%.
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
+ Coverage   78.94%   79.39%   +0.44%     
==========================================
  Files         525      532       +7     
  Lines        9067    11529    +2462     
  Branches       26       27       +1     
==========================================
+ Hits         7158     9153    +1995     
- Misses       1909     2376     +467     
Files Coverage Δ
...cala/lucuma/odb/graphql/input/GroupEditInput.scala 100.00% <ø> (ø)
.../lucuma/odb/graphql/mapping/GroupEditMapping.scala 100.00% <100.00%> (ø)
...ma/odb/graphql/predicate/GroupEditPredicates.scala 100.00% <ø> (ø)
...in/scala/lucuma/odb/graphql/topic/GroupTopic.scala 77.41% <66.66%> (ø)
...a/lucuma/odb/graphql/predicate/LeafPredicate.scala 54.54% <0.00%> (-20.46%) ⬇️
...cuma/odb/graphql/mapping/SubscriptionMapping.scala 65.09% <85.71%> (+11.76%) ⬆️

... and 46 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@swalker2m swalker2m left a comment

Choose a reason for hiding this comment

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

This looks good / makes sense to me. Is it still a WIP?

@tpolecat
Copy link
Member Author

It's ready, assuming it does what Hugo needs. I want to double-check with him before merging.

@hugo-vrijswijk
Copy link
Contributor

hugo-vrijswijk commented Nov 1, 2023

I think this is good. Ideally there would be three events (or 2 when moved inside the same group)

  • groupEdit if a group is moved (with parentGroupId and/or parentGroupIndex changed) (already fired, I think)
  • observationEdit when an obs is moved (with groupId and/or groupIndex changed) (already fired, I think)

And for the old/new groups

  • groupEdit for the new group (with new elements)
  • groupEdit for the old group (with changed elements
    With this being a single event if an element is moved inside the same group.

For the root group it works if there is no event for the root group, only for the new group (and for elements in the root group being moved/updated). I.e:

  1. Obs is moved outside of root group
  2. observationEdit event with a new groupId (already works)
  3. groupEdit event for the group the obs is moved to
  4. observationEdit with groupIndex updated for the elements in the root group (already works, I think)

"""
value: Group!
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this has to be nullable. If the root group is updated it's fine if just the elements inside it are updated (with a new index)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for consistency it needs to be nullable, but you can ignore this case on your end if you like.

@tpolecat tpolecat changed the title wip: improve group events improve group events Nov 2, 2023
@tpolecat
Copy link
Member Author

tpolecat commented Nov 2, 2023

Ok I guess let's merge this and see how it goes. We can continue to tweak it depending on what Hugo runs into.

@tpolecat
Copy link
Member Author

tpolecat commented Nov 2, 2023

Need an amen from someone.

Copy link
Contributor

@swalker2m swalker2m left a comment

Choose a reason for hiding this comment

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

Amen.

@tpolecat tpolecat merged commit cc8f3e4 into main Nov 2, 2023
6 checks passed
@tpolecat tpolecat deleted the more-group-events branch November 2, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants