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

Articles API: filtering by tags (#3654) #10614

Merged
merged 3 commits into from Oct 9, 2020

Conversation

rafaltrojanowski
Copy link
Contributor

@rafaltrojanowski rafaltrojanowski commented Oct 5, 2020

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

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Closes #3654

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, as well
as any relevant images for UI changes.

Added tests?

  • yes
  • no, because they aren't needed
  • no, because I need help

Added to documentation?

  • docs.forem.com
  • readme
  • no documentation needed

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

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

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 5, 2020
@CLAassistant
Copy link

CLAassistant commented Oct 5, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Hi @rafaltrojanowski, thanks for taking the first stab at this. The functionality seems to be working correctly! I would add a test to explictly state that if you pass, for example, "java" in the included tags and "java" in the excluded tags, you'll get back nothing :)

I played a bit with it locally and it works. A few details:

ps. don't foget to name the branch rafaltrojanowski/something the next time ;)

spec/services/article_api_index_service_spec.rb Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 6, 2020
@rafaltrojanowski rafaltrojanowski requested a review from a team October 6, 2020 10:30
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 6, 2020
* Add test case
* Move tests
* Update docs
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Thanks for the change @rafaltrojanowski!

The only thing we have to decide if tags should be in AND like they are now, or in OR.

See the discussion here #3654 (comment)

cc @citizen428 @Ridhwana

@benhalpern
Copy link
Contributor

@rhymes what if we took the comma-separated version as meaning AND (like here), and allowed pipes (|) to indicate OR (which doesn't allow us to do more complicated boolean logic, but that's probably fine)....

@citizen428
Copy link
Contributor

Gut feeling I'd go with OR as the default, but as long as it's documented I can live with either.

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

This looks good to me. Sensible default, properly documented. ✅

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 8, 2020
@benhalpern benhalpern requested a review from rhymes October 8, 2020 14:53
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks @rafaltrojanowski!

@rhymes rhymes merged commit 2a488a1 into forem:master Oct 9, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 9, 2020
citizen428 pushed a commit that referenced this pull request Oct 12, 2020
* Articles API: filtering by tags (#3654)

* Address CR comments (#3654)

* Add test case
* Move tests
* Update docs

* Use _any_ option for querying by tags (#3654)
boardfish pushed a commit to boardfish/forem that referenced this pull request Oct 17, 2020
* Articles API: filtering by tags (forem#3654)

* Address CR comments (forem#3654)

* Add test case
* Move tests
* Update docs

* Use _any_ option for querying by tags (forem#3654)
benhalpern added a commit that referenced this pull request Oct 20, 2020
* chore: update the profile to be more dynamic. Maps out the group + field name   + value for field

* feat: add the correct input type to the form tying it to the correct method to update

* feat: update the value and the label

* feat: update the profile with a submit

* refactor: ensure we dont show empty groups on the settings/profile

* fix: this shoudl be dribbble

* fix: use attributes! to refresh the cache to ensure that we font run into the case where Profile.attributes is []

* feat: show group description only when it exists

* chore: add some classes

* feat: use the profile update service to update the relevant user information

* chore: rename variable

* chore: use a scope to not return empty groups

* fix: only update user if the profile sync is update is successful. Also capture errors

* feat: style the crayons field checkbox

* refactor: rename the attribute to user instead of profile

* refactor: service object

* Fix CodeClimate warning and rearrange code

* Fix service object, add missing attribute

* Fix flash and add redirect

* Fix service object + spec

* Fix user edits profile spec

* Disable spec

* Fix more specs

* Undo vim fail

* Add data update scripts

* Update where clause in data update script

* Rename method

* Make sync_to_user conditional

* Add after update actions and Honeycomb

* feat: handle checkbox allow chcekbox to check

* Don't set experience cookie in ProfilesController

* Clean up Profiles::Update object

* Fix spec

* Remove Twitch integration (#10732)

* Remove Twitch integration

* Ignore columns

* Change admin password in docs (#10735)

Update default admin password in documentation

* Do not require meta_keywords to be set (#10721)

* Remove extra character from meta_keywords in /listings/index.html.erb

* Remove meta_keywords from MANDATORY_CONFIGS

* Add and use meta_keywords_tag helper

* Use modern tag syntax instead of deprecated syntax

* Add and use meta_keywords_default helper

* Add and use meta_keywords_article helper

* Remove * from meta_keywords_field.label

* Update meta_keyword specs to account for no keywords being set

* Generalizes Connect welcome message and settings nav tab and refactors tests (#10741) [deploy]

* [deploy] fix: Clear chat input after Enter key submit (#10487)

* fix: Clear chat input after Enter key submit

* fix: Rename Composer component

* test: Add integration tests for input value

* After pressing Enter
* After clicking Send
* After clicking Save edit
* After clicking Close edit

* Remove tabindex of -1 on upload image input in the write a post editor (#10543)

* Remove tabindex of -1 on upload image input in the write a post editor

* Remove negative tabindex on image uploader's outer button

* Give tabindex of -1 to outer button on image uploader

* Update buttons.scss

* [deploy] Add recaptcha keys to as site-configurable keys (#10736)

* Add recaptcha keys to as site-configurable keys

* Add recaptcha to site config view

* Use site config key

* Make ReCAPTCHA tag optional if keys are blank

* Use proper capitalization of recaptcha

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Refactor logic for readability

* Use proper capitalization of recaptcha

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Use proper capitalization of recaptcha

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Use proper capitalization of recaptcha

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Add Section for Series/Collections (#10719)

* Add Section for Series/Collections

I added a small section in this area of the docs to share some information about collections/ series. 

There is not much information in the documentation about collections or series and I felt like this would be beneficial to others looking to help with the Series feature of the website.

* Updated wording of collections section.

* Articles API: filtering by tags (#3654) (#10614)

* Articles API: filtering by tags (#3654)

* Address CR comments (#3654)

* Add test case
* Move tests
* Update docs

* Use _any_ option for querying by tags (#3654)

* Move Imgproxy endpoint config to Images::Optimizer (#10742)

* Link Email Account creation UI to existing Create Account UI (#10746) [deploy]

* Conditional button to link to email accout creation

* Rename email signup state param

* Test for email account creation button scenarios

* Add forem meta tags (#10747)

* Add forem meta tags

* Add docs

* Update docs/technical-overview/compatibility.md

Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com>

* Update docs/technical-overview/compatibility.md

Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com>

* Update docs/technical-overview/compatibility.md

Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com>

* Update docs/technical-overview/compatibility.md

Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com>

* Remove Twitch-related columns from users table (#10750)

* [deploy] Long text goes outside cards (#10740)

* Add word-break to crayons-card

Add word-break to crayons-card this will allow words to be broken if the word is too long but will wrap normally otherwise.

* Use overflow-wrap: anywhere;

`break-word` is deprecated though `anywhere` does not work in IE

* Removes superfluous sign_in from users_onboarding_spec.rb (#10757) [deploy]

* [deploy] Allow glitch id starting with ~ (#10544)

* [deploy] crayons prep (#10737)

* Add DEV special case for following hiring tag

* Move image validation to Profiles::Update object

* spec: move over the tests for validating images

* Move follow_hiring_tag into the service object

Co-authored-by: Ridhwana <ridhwana.khan16@gmail.com>
Co-authored-by: Andrew Bone <AndrewB05@gmail.com>
Co-authored-by: Vaidehi Joshi <vaidehi.sj@gmail.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Robin Gagnon <me@reobin.dev>
Co-authored-by: Manaswini <58681405+Manaswini1832@users.noreply.github.com>
Co-authored-by: Andy Zhao <17884966+Zhao-Andy@users.noreply.github.com>
Co-authored-by: Christopher Wray <53663762+cwray-tech@users.noreply.github.com>
Co-authored-by: Rafal Trojanowski <rt.trojanowski@gmail.com>
Co-authored-by: Mac Siri <mac@dev.to>
Co-authored-by: Josh Puetz <josh@dev.to>
Co-authored-by: Ben Halpern <bendhalpern@gmail.com>
Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com>
Co-authored-by: irmela <irmela@berlin-coding.de>
Co-authored-by: ludwiczakpawel <ludwiczakpawel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Articles API: filtering by tags
5 participants