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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 6.22%. Comparing base (e4bed74) to head (ed9f3a2).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
app/routes/circulars/circulars.server.ts 0.00% 5 Missing ⚠️
app/email-incoming/circulars/index.ts 0.00% 3 Missing ⚠️
app/routes/synonyms/synonyms.server.ts 25.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #2642      +/-   ##
========================================
- Coverage   6.22%   6.22%   -0.01%     
========================================
  Files        167     167              
  Lines       4222    4242      +20     
  Branches     466     469       +3     
========================================
+ Hits         263     264       +1     
- Misses      3957    3976      +19     
  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.

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