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.

Sign up for GitHub

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

Jump to bottom

CV2-3777: expose TiplineRequestType #1692

Merged
merged 11 commits into from
Oct 19, 2023
Merged

CV2-3777: expose TiplineRequestType #1692

merged 11 commits into from
Oct 19, 2023

Conversation

melsawy
Copy link
Contributor

@melsawy melsawy commented Oct 10, 2023 •
edited
Loading

Description

  • Expose tipline request type in GraphQL
  • Added a new fields report_sent_at , report_correction_sent_at & smooch_request_type

References: CV2-3777

How has this been tested?

Implemented automated tests.

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

  • File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
  • Etc.

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

caiosba
caiosba reviewed Oct 11, 2023
View reviewed changes
app/graph/types/query_type.rb Outdated
Comment on lines 205 to 211
field :tipline_request,
TiplineRequestType,
description:
"Information about tipline request, given its id",
null: true do
argument :id, GraphQL::Types::String, required: true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Check permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used same permission for dynamic_annotation_field calling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that permission only applies to global API keys, so it won't work from the web client for regular users, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this API endpoint anyway? We always we want to get the requests under the scope of an item, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just talked to Sawy - we can delete this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted

caiosba
caiosba reviewed Oct 11, 2023
View reviewed changes
app/models/bot/smooch.rb Outdated
self.send_report_to_user(uid, data, pm, lang, 'fact_check_report')
end
unless field_name.blank?
a = pm.get_annotations('smooch').last.load
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of getting the annotation here, please pass it as a parameter to the method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@melsawy melsawy marked this pull request as ready for review October 17, 2023 04:15
@melsawy melsawy requested a review from DGaffney as a code owner October 17, 2023 04:15
@melsawy melsawy requested a review from caiosba October 17, 2023 04:15
caiosba
caiosba reviewed Oct 17, 2023
View reviewed changes
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.

Just added two comments, Sawy. Also, please check if the frontend still works with these changes. If not, let's wait to merge this branch until the backend changes are in place too.

app/graph/types/query_type.rb Outdated
Comment on lines 205 to 211
field :tipline_request,
TiplineRequestType,
description:
"Information about tipline request, given its id",
null: true do
argument :id, GraphQL::Types::String, required: true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that permission only applies to global API keys, so it won't work from the web client for regular users, right?

app/graph/types/query_type.rb Outdated
Comment on lines 205 to 211
field :tipline_request,
TiplineRequestType,
description:
"Information about tipline request, given its id",
null: true do
argument :id, GraphQL::Types::String, required: true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this API endpoint anyway? We always we want to get the requests under the scope of an item, no?

@melsawy melsawy requested a review from caiosba October 19, 2023 04:33
caiosba
caiosba approved these changes Oct 19, 2023
View reviewed changes
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.

Sawy, please just add a test for the new field:
Unconvered lines in app/models/concerns/smooch_fields.rb: 81, 82
Good to merge after that.

@codeclimate
Copy link

codeclimate bot commented Oct 19, 2023

Code Climate has analyzed commit dce137f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

@melsawy melsawy merged commit 29f6be3 into develop Oct 19, 2023
4 checks passed
@melsawy melsawy deleted the CV2-3777-expose-tipline-request-type-and-report-delivery-state-in-graph-ql-api branch October 19, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caiosba caiosba caiosba approved these changes

@DGaffney DGaffney Awaiting requested review from DGaffney DGaffney is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@melsawy @caiosba