Add an article exclusion list to Billboards #19280

jaw6 · 2023-03-30T08:08:40Z

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Closes #Add an article exclusion list to Billboards #19226

QA Instructions, Screenshots, Recordings

https://www.loom.com/share/aa40a1d49ff949f590012d7268e999c3

Added/updated tests?

Yes
No, and this is why:
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs or
Storybook (for Crayons components)
This PR changes the Forem platform and our documentation needs to be
updated. I have filled out the
Changes Requested
issue template so Community Success can help update the Admin Docs
appropriately.
I've updated the README or added inline documentation
I've added an entry to
CHANGELOG.md
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

spec/models/display_ad_spec.rb

spec/requests/api/v1/docs/display_ads_spec.rb

codecov · 2023-03-30T08:22:29Z

Codecov Report

Patch coverage: 96.55% and project coverage change: +0.01 🎉

Comparison is base (a603002) 88.73% compared to head (0b992c3) 88.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19280      +/-   ##
==========================================
+ Coverage   88.73%   88.75%   +0.01%     
==========================================
  Files        1175     1175              
  Lines       26389    26413      +24     
  Branches     2027     2028       +1     
==========================================
+ Hits        23417    23442      +25     
+ Misses       2818     2817       -1     
  Partials      154      154

Flag	Coverage Δ
cypress	`69.91% <94.11%> (+0.08%)`	⬆️
javascript	`76.17% <94.11%> (+0.05%)`	⬆️
jest	`46.30% <0.00%> (-0.09%)`	⬇️
ruby	`94.74% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
app/controllers/api/v1/display_ads_controller.rb	`93.93% <ø> (ø)`
app/javascript/packs/admin/displayAds.jsx	`87.75% <94.11%> (+2.04%)`	⬆️
app/models/display_ad.rb	`100.00% <100.00%> (ø)`
app/queries/display_ads/filtered_ads_query.rb	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-03-30T08:35:07Z

Uffizzi Preview deployment-21353 was deleted.

app/models/display_ad.rb

spec/requests/api/v1/docs/display_ads_spec.rb

benhalpern · 2023-03-30T18:06:31Z

Overall LGTM. I'll hold off from an official review to make sure this gets plenty of additional eyeballs.

Co-authored-by: Ben Halpern <bendhalpern@gmail.com>

Ridhwana

@jaw6 this looks good and sound to me ✨ Thanks! Left two small bits of feedback.

spec/requests/api/v1/docs/display_ads_spec.rb

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

klardotsh

I was auto-requested for review here, but don't have the context necessary to really give this a deep business logic review, so consider my approval fairly gentle. My lone comment that can actually be acted on is optional/somewhat of a conversation piece more than anything.

klardotsh · 2023-04-03T22:59:52Z

.rubocop.yml

-  Enabled: true
-  Max: 15
-  StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExampleLength
+  Enabled: false


Nit: this diff, in my mind, feels like a separate PR to some degree, but I also acknowledge that this becomes Rebase Hell after a while, so I'm somewhat leaving this comment to learn the team philosophy around stuff like this.

(I see it's already its own commit with 85ed451, so git cherry-pick onto a new branch, and maybe git commit --amend to do some rewording, and that could be made standalone)

klardotsh · 2023-04-03T23:02:46Z

spec/requests/api/v1/docs/display_ads_spec.rb

-          },
-          required: %w[name body_markdown placement_area]
-        }
+        parameter name: :display_ad, in: :body, schema: { type: :object,


Nothing to do with your work in this file, just the fact that this file exists at all is pretty neat - I love this "autogenerate the JSONSchema from real enum hashes" approach.

…le_exclusion * origin/main: [ruby] Update all development Bundler dependencies (2023-04-03) (#19291) [CI] Make CodeCov less noisy (#19286)

benhalpern

Gave this another pass, it all looks good, and verified the functionality locally and in Uffizzi.

jaw6 added 5 commits March 30, 2023 09:36

Add exclude_article_ids int array

75dc878

Add specs, normalize inputs better

99ca65c

Add to form & controller

ef7532b

Add exclude_article_ids to DisplayAd API

14a969f

Use exclude_article_ids in query

d9a61db

jaw6 requested review from a team as code owners March 30, 2023 08:08

jaw6 requested review from klardotsh, Ridhwana and lboogie04 and removed request for a team March 30, 2023 08:08

github-actions bot reviewed Mar 30, 2023

View reviewed changes

spec/models/display_ad_spec.rb Show resolved Hide resolved

spec/requests/api/v1/docs/display_ads_spec.rb Outdated Show resolved Hide resolved

jaw6 added 5 commits March 30, 2023 14:32

Rubocop

819aca7

Comment typo

bafc978

Tweak rspec example length config

85ed451

Arguments all the way down

05d5512

Typo

e3c6260

benhalpern reviewed Mar 30, 2023

View reviewed changes

app/models/display_ad.rb Show resolved Hide resolved

benhalpern reviewed Mar 30, 2023

View reviewed changes

spec/requests/api/v1/docs/display_ads_spec.rb Outdated Show resolved Hide resolved

jaw6 and others added 2 commits March 31, 2023 09:04

Update spec/requests/api/v1/docs/display_ads_spec.rb

c1ba42a

Co-authored-by: Ben Halpern <bendhalpern@gmail.com>

Merge branch 'main' into jaw6/display_ad_article_exclusion

19b03fd

Ridhwana reviewed Mar 31, 2023

View reviewed changes

spec/requests/api/v1/docs/display_ads_spec.rb Outdated Show resolved Hide resolved

jaw6 and others added 2 commits March 31, 2023 11:49

Update spec/requests/api/v1/docs/display_ads_spec.rb

2fff253

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

Swagger schema for Display Ad

8f5acff

jaw6 added the merge by any core This tag is a signal that you are okay with any core team member merging your code for you. label Mar 31, 2023

Merge branch 'main' into jaw6/display_ad_article_exclusion

29ec75a

Merge branch 'main' into jaw6/display_ad_article_exclusion

fbc949e

klardotsh approved these changes Apr 3, 2023

View reviewed changes

jaw6 added 2 commits April 4, 2023 08:57

Merge remote-tracking branch 'origin/main' into jaw6/display_ad_artic…

7a89fa0

…le_exclusion * origin/main: [ruby] Update all development Bundler dependencies (2023-04-03) (#19291) [CI] Make CodeCov less noisy (#19286)

Merge branch 'main' into jaw6/display_ad_article_exclusion

0b992c3

benhalpern approved these changes Apr 5, 2023

View reviewed changes

benhalpern merged commit d3f8e12 into main Apr 5, 2023

benhalpern deleted the jaw6/display_ad_article_exclusion branch April 5, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an article exclusion list to Billboards #19280

Add an article exclusion list to Billboards #19280

jaw6 commented Mar 30, 2023

codecov bot commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

benhalpern commented Mar 30, 2023

Ridhwana left a comment

klardotsh left a comment

klardotsh Apr 3, 2023

klardotsh Apr 3, 2023

benhalpern left a comment

Add an article exclusion list to Billboards #19280

Add an article exclusion list to Billboards #19280

Conversation

jaw6 commented Mar 30, 2023

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added/updated tests?

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

github-actions bot commented Mar 30, 2023 • edited Loading

benhalpern commented Mar 30, 2023

Ridhwana left a comment

Choose a reason for hiding this comment

klardotsh left a comment

Choose a reason for hiding this comment

klardotsh Apr 3, 2023

Choose a reason for hiding this comment

klardotsh Apr 3, 2023

Choose a reason for hiding this comment

benhalpern left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading