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

create synonym group for eventId if none exists #2642

Merged
merged 6 commits into from
Nov 15, 2024

Conversation

Courey
Copy link
Contributor

@Courey Courey commented Oct 22, 2024

Description

This change adds functionality to check if there is an existing synonym group for an eventId upon circular creation. If there is an eventId and no synonym group for that eventId, it creates one. If there is no eventId it skips. If there is an eventId and the synonym group already exists for that eventId it skips.

Related Issue(s)

Resolves #2661

Testing

I tested this locally.
However, because it is in a critical area, we need to thoroughly test it on dev.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.

Project coverage is 6.21%. Comparing base (e4bed74) to head (eb2ce74).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
app/routes/circulars/circulars.server.ts 0.00% 4 Missing ⚠️
app/routes/synonyms/synonyms.server.ts 42.85% 4 Missing ⚠️
app/email-incoming/circulars/index.ts 0.00% 2 Missing ⚠️
app/routes/synonyms.new.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #2642      +/-   ##
========================================
- Coverage   6.22%   6.21%   -0.02%     
========================================
  Files        167     167              
  Lines       4222    4245      +23     
  Branches     466     471       +5     
========================================
+ Hits         263     264       +1     
- Misses      3957    3979      +22     
  Partials       2       2              

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

@lpsinger
Copy link
Member

Why is this necessary? Is it because, currently, the grouped view does not show events that do not have an entry in the synonyms table?

@Courey
Copy link
Contributor Author

Courey commented Oct 22, 2024

Why is this necessary? Is it because, currently, the grouped view does not show events that do not have an entry in the synonyms table?

yup. Judy wanted to all events to have an entry, not just the manually created ones.

@Courey
Copy link
Contributor Author

Courey commented Oct 22, 2024

And in addition to this change, I am aware that there needs to be a backfill to get things to work. I need a backfill to populate the eventId in circulars that existed before I added in the eventId on circular creation. I then need a backfill to populate the synonyms with the existing eventIds.

@lpsinger
Copy link
Member

This code contains a race condition.

@Courey
Copy link
Contributor Author

Courey commented Oct 22, 2024

okay. I'll not make draft PRs for things I'm still working on in the future. This is not ready for review at all.

@lpsinger
Copy link
Member

okay. I'll not make draft PRs for things I'm still working on in the future.

I wouldn't say that! Anyone is always welcome to make draft PRs.

This is not ready for review at all.

I wasn't offering a review; I was just asking for the motivation for this PR, since I missed the Monday call.

@Courey Courey marked this pull request as ready for review November 1, 2024 15:33
@Courey Courey requested review from lpsinger and dakota002 and removed request for lpsinger November 4, 2024 15:35
Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

Please use TransactWriteItems to eliminate the race condition.

app/routes/synonyms/synonyms.server.ts Outdated Show resolved Hide resolved
@Courey
Copy link
Contributor Author

Courey commented Nov 14, 2024

I made a couple of updates here.

  1. instead of a hard delete when a moderator removes a synonym from a group, it now will just assign the removed one a unique id to maintain the expectation that all events will have a synonym group.
  2. Instead of deleting a synonym group when it is deleted, each member will be given a new uuid so they all have their own synonym group.
  3. the initial creation of a synonym has different rules than one that is created by a moderator. Instead of using a transaction (which costs 2x), I could just get away with using a condition check on the put and swallowing the failure if it fails that check. That enabled me to remove any check for existing synonyms and avoid more expensive transactions.
  4. I re-named the createSynonyms function to moderatorCreateSynonyms since the behavior differs depending on how the synonym is created. This ensures that developers will know to distinguish between moderator created and system initiated.

@Courey Courey requested a review from lpsinger November 14, 2024 20:37
@lpsinger lpsinger merged commit 37f18ac into nasa-gcn:main Nov 15, 2024
12 checks passed
@Courey Courey deleted the courey/make_all_events_have_a_group branch November 15, 2024 15:43
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.

create synonym for new circulars automatically if one does not already exist
2 participants