The hotness score is an "opaque to our users" score, it reflects
"activity" on the tag. However, by also showing the number of tags on
the associated view, there's a bit of a head scratcher when a "hotter"
tag with less tags is rendered ahead of a popular tag.
Closes#16321
* Added tag search to nav menu
* Added tag search
* Improved tags search results view
* Removed commented lines from the controller
* Fix specs for Search::Tag
* Prepare for tags search pagination
* Fixed Search::Tag specs
* styling
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* Convert symbol hash keys to strings when calling .perform_async
Fixes a warning from Sidekiq 6.4.0+ about perform_async arguments
which are not equal when passed to perform (`JSON.parse(JSON.dump(arg))`
should equal arg).
This is a safety measure to prevent passing objects (like classes, or
model instances) rather than their representations (like a class name,
or a model's attributes hash).
This warning will be an error in sidekiq 7
* Turn warning into an error in non-production environments
* Use string keys for reaction notification and article fetched
Missed these two on the first pass
* Update example argument hashes for #enqueues_on_correct_queue
Since this calls perform_async under the hood we need to pass json
safe hashes in the test cases as well.
https://github.com/forem/forem/blob/main/spec/workers/shared_examples/enqueues_on_correct_queue.rb
* Convert keys from FollowData#to_h to string before perform_async
I'm not sure enough where else (outside of notification) to_h is being
called, so I'm converting here when building args, rather than in
FollowData#to_h, which might be my next step.
* Let FollowData#to_h return a hash with string keys
Update spec to use string keys as well.
* Make to_h return string keys for ReactionData
Like FollowData, the #to_h method is only used to call
notifications (this is used to enqueue sidekiq jobs).
* Remove a key that was in the hash
Since reaction_data calls to_h, it gets string and not symbol,
keys. Call Hash#except with a key that was actually there.
* Refactoring cached_followed_tags
This refactor consolidates two queries and a loop into a single query.
One thing that is unclear is if we're ever really clearing the cache of
the user by id?
What follows is un-related to the refactor but appears to be related to #14937.
> First, I see we set a cache here:
>
> ebdaaaf15b/app/decorators/user_decorator.rb (L23-L34)
>
> But when I look at what busts the user cache, it doesn’t look like we’re busting it for the above cached location:
>
> ebdaaaf15b/app/services/edge_cache/bust_user.rb (L3-L22)
>
> What if anything am I missing in regards to cache busting?
* Adding some deprecated methods
* Updating documentation
* Bump for travis
* Bump for travis
* Updating comments to fix confusion
NOTE: I might be introducing more confusion, but at least
I'm addressing the prior confusion.
* Bump for travis
* Updating so we don't cache ActiveRecord objects
* Selecting only applicable attributes for caching
* Apply suggestions from code review
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Adjusting code based on feedback
* Bump for travis
* Bump for travis
* Update app/decorators/user_decorator.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Don't set saw_onboarding automatically
This should be set by the onboarding checkbox update (user accepted
terms of service and code of conduct).
The issue is a PATCH request to track the last seen page is sent on
the first page, _before_ the user consents to the terms via the
checkboxes.
This issue is masked by a 422 unprocessable entity response when we
don't get a username in the patch request (next commit).
* Only validate username on the username page
Every other "last page seen" dialog was returning a 422 because the
username is only submitted in the 3rd dialog "build your profile".
Only validate the username field when it's submitted, process other
onboarding updates normally.
* Remove failing test
Since the onboarding_update action no longer sets saw
onboarding (since we send the patch before they've accepted the code
of conduct), don't test that we do.
There's a parallel test for the onboarding checkbox update later in
this file that covers the checkboxes have been accepted.
* overwrite instead of merging user_params
We now initialize it empty, no need to merge params here.
* Let override of ToS and CoC show in onboarding
Prior to this commit any changes to the terms or code of conduct were
not reflected in the onboarding links.
With this commit, I'm leveraging a newly created method that first
checks if there's a page for the given slug. If there is, use that
page's copy.
To test:
- Overwrite default /terms page on a Forem
- Navigate to /onboarding or sign up as a new user
- Click on Terms of Use link text
- Review Terms of Use
Closes#15296
* Adding spec around not passing a block
* Remove special handling of "connect" feedback by name
The special casing was related to "connect" feedback having both a
reporter and an offender. Check for offender instead.
Additionally, there was special casing in the controller to rate-limit
connect feedback separately from other channels. Since connect doesn't
exist, we should not need this.
There's a small bit of functionality (when I post to feedback_messages, the
number of feedback messages increases) that was removed from the test
case, we can add that back (and "connect" type, and
offender_id attributes) since it looks like it might have been a
useful assertion.
* Add back feedback message controller creates feedback message case
This was removed in the last commit because it was in a "connect" chat
channel context, but the basic "should persist a record" test was
otherwise valid. Submit an abuse-report rather than a connect message
report.
* typo
feeedback, woops.
* Ensuring we don't track views of author or unpublished
Prior to this commit, I was surprised to learn that we:
1) Tracked an author's view of their article.
2) Tracked views of an unpublished article.
This came up from a Forem creator asking if they could reset the view
counter. Or trigger the reset on publication.
I think a general business logic policy of don't track views for the
author and don't track views for unpublished articles is a reasonable
default.
Were we to pursue the clear views on publication, we'd need to consider
something that went from unpublished -> published -> unpublished ->
published. Without a more explicit state machine, triggering a
busineiss logic behavior seems a bit unexpected.
In other words, I wrote an article. There are 20 views when I realize
that I need to unpublished it. I make the changes in the unpublished
state, and re-publish. I'd assume that those 20 views would still be
"recorded" and counted towards my article's view counts.
* Adjusting condition structure
Prior to this commit, the `if` clause was rather far to the right. This
helps make the if clause more pronounced.
* Adding documentation and constant
This loosely relates to a request for information from Success team
about whether or not viewing a draft article counts towards it's stats.
* Apply suggestions from code review
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Refactoring to add helper method
Prior to this commit, we made view level calls to service modules. This
refactor provides convenience methods on the model.
Furthermore, it addresses a few Rubocop violations that "come along for
the ride."
* Ensuring cached entity squaks like User
* Fixing broken spec
* Fixing typo
* Initial work
* Implement strategy injection into WeightedQueryStrategy
* Modify field_test config and adding variants
* Change orginal to constant and make some other adjustments
* Fix hardcoded test values
* Remove all references to VerifySetupCompleted to remove setup banner
* Removes setup banner-related specs
* Adjusts the title in activateMissingKeysModal
* Adds the show expand classes to getStartedBodyContainer in show.html.erb
* Tidying up and documenting Tag model
Prior to this commit we had a custom `where(alias_for: [nil, ""])`
call. That call highlighted that we lacked a term for a Tag that was
not an alias. As part of this commit, I named that a "concrete" tag.
Further, I added scopes to assist in helping "name" those concepts.
This commit also adds a data migration and utilization of
StringAttributeCleaner to hopefully get away from `alias_for == ""`
situations.
As of writing this commit <2022-01-04 Tue 17:22 UTC>, in DEV.to we had 5
tags with `alias_for == ""`:
- actionshackathon21
- regex
- atlashackathon
- hotwire
- foremfest
In https://dev.to/admin/blazer I ran the following:
```sql
SELECT name FROM tags WHERE alias_for = ''
```
* Renaming concrete to direct
* Skip google analytics when ga_tracking_id is nil
This commit involves short-circuiting Google Analytics calls when the
Forem has not configured a `Settings::General.ga_tracking_id`. Note,
depending on your local `.env` file (or configured ENV variables) you
may have a `GA_TRACKING_ID` value set; I did, it was set to "Optional"
which overrode the database setting.
We set the HTML data properites in two places: [admin.html.erb][1] and
[application.html.erb][2].
In addition, I'm short-circuiting the local fallback analytics call (via
[Stacato][https://github.com/tpitale/staccato]). My understanding of
Stacato, based on a cursory read, requires a Google Analytics Tracking
ID to work.
And last, we have a one off of javascript for Google Analytics tracking.
This closes#15962.
[1]:528bd2baa6/app/views/layouts/admin.html.erb (L31)
[2]:528bd2baa6/app/views/layouts/application.html.erb (L55)
How to test?
- Check your .env file to see if you have set GA_TRACKING_ID. If so,
unset it.
- Start with a fresh Forem instance.
- Open your browser and open the developer tools to inspect the Network
activity.
- Open the homepage of your local Forem instance (http://localhost:3000)
- Filter your Network results for analytics. You shouldn't see any.
Also, make sure you're disabling any blockers you might have as that
influences things.
* Commenting out GA_TRACKING_ID
Related to https://github.com/forem/forem/pull/15967
* Favor empty GA_TRACKING_ID env variable
Prior to this commit, we had a couple of references to magic strings
regarding DisplayAdEvent objects. This commit seeks to consolidate that
behavior.
In a handful of cases we had javascript variables, but as these JS files
are `erb` templates, we can inject the constant as well. This reduces
repetition of knowledge.
* add needed fields to tag search
* specs
* add badge to tags suggest response (#15840)
* Add badge to tags suggest response
* Update tags_spec.rb
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
* limit tag search badge to only badge_image
* only unpack badge image if badge exists
Co-authored-by: Dwight Scott <dwight@forem.com>
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
* Proposing a new feed experiment
This commit involves tweaking a few subtle aspects of the challenger:
1) Remove the sort by published date on the relevance score. The
articles will now be listed in the order of perceived relevance.
2) Disable a few levers that did little. There really aren't any
`articles.featured = true` articles in the database.
3) Gently increase the weight of each comment in a linear manner.
4) Give a little more weight to posts that don't have followed tags.
In addition, it involves renaming the conversions to better convey their
implementation. This renaming helps create more descriptive labels for
the test results at `/admin/abtests`.
This commit also adds logic to repurpose the AbExperiment feed_strategy
logic; I envision adjusting the weighted feed strategy levers with some
frequency.
Per discussions, I'm also disabling the "not logged in" feed testing.
We'll use the LargeForemExperimental for this.
* Flipping attribute name to positive
Mentally "not disabled" is harder to parse than "enabled". This change
helps with setting an optimistic attribute.
* Bump for travis
* Bump for travis
* Removing file committed by mistake
* Extracting method
* Refactoring to consolidate logic
Prior to this commit, two controllers had nearly identical chunks of
logic. This refactor extracts the logic to a common and more canonical
location.
* Addressing rubocop's aggressive auto-fix
* Adding spat operator for pluck
* Renaming method for greater clarity
* Refactoring to leverage an Article scope
I've been looking at the queries that involve filtering articles based
on scores. There's several places that seem to be close to consistent,
but have some nuanced difference.
This refactor consolidates one of those "almost the same" cases.
* Bump for travis
* Add missing spaces
* Use filter_map over map + reject/compact
* Simplify FactoryBot calls
* Use to_h with block instead of map + to_h
* Use guard clause
The 400 and 403 error pages show json parsing errors in most browsers,
since "Error: Bad Request" is not a valid json body ('"Error: Bad
Request"' would be, or the object with key error and value of the
string which I've selected is _also_ valid).
Do we have frontend code that's looking for this body before it parses
for some reason, or was this just a mistaken copying from the api
controller in the initial PRs (#2293 for not_authorized, and #6248 for
the bad_request method, which may have just replicated the decision
for not_authorized)?
* Refactoring questions asked of user
In this pull request, I'm extracting and normalizing role-based
questions asked of the user.
Prior to this commit, our codebase has asked two very similar questions
of our user model:
- `user.has_role?(:admin)`
- `user.admin?`
In asking `has_role?(:admin)` we are relying on implementation details
of the rolify gem. In addition, the `has_role?` question asked
throughout controllers or views means that it's harder to create
hieararchies of permissions.
In favoring `user.admin?` as our question, we can use that indirection
as an opportunity to discuss and decide "Should someone with the
`:super_admin` role be `user.admin? == true`?"
The details of this commit is to do three primary things:
1. Ask the `has_role?` questions in "one place" in the code (e.g. the
`Authorizer` module)
2. Extract the role based questions that are on the `User` model and
provde backwards compatable delegation.
3. Structure the code so that it's harder to accidentally call
`user.has_role?` (e.g., make `User#has_role?` and `User#has_any_role?`
private).
This is related to #15624 and the updates are informed by discussion in
PR #15691. This commit supplants #15691.
* Refactoring the liquid tag policy tests
* Fixing typo
* Bump for travis
* Extracting container for allowed tags & attrs
Prior to this commit, we had several different locations in which we
specified ALLOWED_TAGS and ALLOWED_ATTRIBUTES for HTML rendering and
sanitization.
Curious to see how these either intersected or didn't, I opted to
create a container module that allows for us to more readily normalize
these allowed tags and attributes. It's possible that we won't do any
normalization, but this work helps make that easier.
Ideally, I'd love us to contextualize "why did we choose the
tags/attributes we chose?" But for now, I think consolidating these
tags and attributes will help make adding a `details` and `summary` tag
easier.
This relates to forem/rfcs#296
See [Google Sheet][1] for analysis of what tags/attributes are used, the
intersection and union.
[1]:https://docs.google.com/spreadsheets/d/1yj-a1qus1o0o4cj-_gOMP5yteeg-_f3s5z7kvK0Y7RM/edit#gid=0
* Fixing misnamed constant
* Fixing misnamed constant
* Extracting additional HtmlRendering use cases
* Adding comparative documentation for HTML tags
* Fixing broken parameter signature
* Moving constants into MarkdownProcessor
* Refactoring away from instance variables
Having both `@tag` and `@tag_model` as variable names can be confusing.
Given that in the prior implementation `@tag_model.name == @tag`, I
figured I would refactor the controller to remove an instance variable.
In addition, this refactor addresses the temporal coupling of methods;
that is to say we call a method which sets an instance variable then
call another dependent on that instance variable.
Yes, ivars are useful to allow for implicit state. However, by favoring
parameters its easier to notice temporal dependencies in method calls.
This helps ensure that we're calling methods in the right order.
Futher, we have a guard clause in place, so let's avoid setting any
additional instance variables that aren't needed if the guard clause
executes.
Related to #15359
* Update app/controllers/stories/tagged_articles_controller.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Favoring constants and Rails where constructs
Prior to this commit, we had a magic array. That magic array was a
duplicate of the Timeframe constant. Likewise, we had a magic string.
This change removes that duplication.
Furthermore, this change favors the `where(key: range..)` construct
instead of "raw" SQL and parameter substition.
Co-authored-by: Michael Kohl <citizen428@forem.com>
* WIP: add a creatore settings form
* WIP: updat the controller to use the Creator Settings FOREM
* feat: use the creator settings form for the new action
* feat: add some default values for the new action
* a note about form data
* update the initiaize function to set some default values
* feat: update the form to use the model data
* feat: permit adn use the attributes within creator_settings_form
* update the flash error
* refactor: require and permit parameters
* chore: use booleans, set defaults and validate the form
* spec: update all the creator_settings tests
* chore: remove comment
* refactor: use self
* feat: aggregate failures'
* chore: remove the logo uploader in the controller
* refactor: update error handling
* feat: update the wasy the controller handles success and error
* chore: remove the resource errors
* feat: show flash message on new line
* fix: use a redirect so that we can get back to /new
* refactor: pass these values through as they seem to be caching whne setting them as default
* chore: change default values
* spec: update tests
* Fix CreatorSettingsForm specs
* fix: use a boolean for public
* spec: add another test for the success var
* fix: radio button labels to correspond + cyress specs
* spec: update based on new changes
* spec: update the params and the expected output
* spec: update the comments and status
* feat: no need for the initialize as we use Active Record Attributes
* feat: update the tac and coc to be persisted when ticked
* fix: amend spec
* blank space
* Message
Co-authored-by: Michael Kohl <me@citizen428.net>
* User decorator (and spec) should use `trusted?`
Fixes a few issues seen in an rspec run that show as:
DEPRECATION WARNING: User#trusted is deprecated, favor
User#trusted? (called from config_body_class at
/opt/apps/forem/app/decorators/user_decorator.rb:58)
And here:
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:112
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:121
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:130
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:139
* prefer User trusted? to trusted
DEPRECATION WARNING: User#trusted is deprecated, favor
User#trusted? (called from permissions at
/opt/apps/forem/app/models/rating_vote.rb:25)
* Prefer trusted? to trusted in user spec
* Use warned? rather than warned in admin article partial
* use trusted? rather than trusted in moderator requests spec
* Prefer trusted? to trusted in moderations controller
* Prefer trusted? to trusted in moderations view
* User auditable? should call trusted? and not trusted
Deprecations go rolling right along.
* Invert guard clause logic to be positive
The original "return unless multiple negated conditions hold" guard
was cumbersome.
Invert to return if any of the exceptions apply, namely:
- this is a comment or readinglist rating (rather than explicit),
allowed for all
- this rating is from a moderator/trusted user (allowed)
- this rating is offered by the article's author (allowed)
I had intended to also remove the safe navigation operators (since it
wasn't clear why there would be a null user or null article, as
rating_vote joins users to articles with a score), but the builtin
validation tests (is expected to validate ...) build objects with
missing attributes, and raise errors when the spec is run.
