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

CV2-5011 refactors for making alegre dual purpose on text encoding #1986

Merged
merged 17 commits into from
Aug 16, 2024

Conversation

DGaffney
Copy link
Contributor

@DGaffney DGaffney commented Aug 5, 2024

Description

In this ticket, I'm adding functionality for Alegre to serve text encoding responses both in the original, soon-to-be-deprecated way, as well as via presto. Idea is we'll serve both for a bit and slowly work out each individual case to use the new system over time. Insofar as check-api is concerned, we're only slightly modifying alegre_v2.rb to extend use to text. Will need to add a few unit tests to accommodate

References: CV2-5011

How has this been tested?

Tested extensively locally, will need to add unit test coverage for new functions.

Things to pay attention to during code review

None in particular

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@DGaffney DGaffney marked this pull request as draft August 5, 2024 17:10
@caiosba caiosba removed their request for review August 5, 2024 21:26
DGaffney and others added 11 commits August 6, 2024 05:59
Bumps [rexml](https://github.com/ruby/rexml) from 3.2.8 to 3.3.3.
- [Release notes](https://github.com/ruby/rexml/releases)
- [Changelog](https://github.com/ruby/rexml/blob/master/NEWS.md)
- [Commits](ruby/rexml@v3.2.8...v3.3.3)

---
updated-dependencies:
- dependency-name: rexml
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Use case: we may need to update the project media information in the Relay store when it's removed from a claim.

Reference: CV2-5000.
* CV2-5002: fix sentry issue

* CV2-5002: fix tests
When a login attempt is successful, we should not increment the counter
for rate limiting.
When trying to create an imported fact-check that had a tag with a trailing space it would fail (`text has already been taken`).

We have a normalization step but that runs `before_validation` on `TagText`.
So when we are checking if the tag exists in line 92 from `tag.rb`, the tag has not been normalized. This means:
- We look for 'tag', it is not there. We create it.
- We look for ' tag', it is not there. We try to create it, it gets normalized, so it becomes 'tag'. Then we fail to create it,
    because it already exists.

Adding the strip when comparing tags fixes this.

Tests
- rails test test/controllers/graphql_controller_12_test.rb:613
- rails test test/models/tag_test.rb:275

References: 5010
PR: 1982
@DGaffney DGaffney marked this pull request as ready for review August 8, 2024 10:24
@DGaffney DGaffney requested a review from caiosba August 8, 2024 10:24
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Devin, please rebase with develop - there are some changes here that are in develop already.

@DGaffney DGaffney requested a review from caiosba August 8, 2024 11:25
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Looks good as WIP. Noting that this handles only ProjectMedia fields.

This information is only needed for the current user, so, it should be under `MeType`, not `UserType`.

References: CV2-4938 and CV2-4704.
@DGaffney DGaffney merged commit 8400e94 into develop Aug 16, 2024
10 checks passed
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.

8 participants