* 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.
* Select max from a subquery of two things
max(int, int) not a function, says postgres, once it stopped yelling
about parenthesis balancing.
* prefer greatest to select max from subquery
* Ensuring that we don't divide by zero
Adding one to a "always-ish greater than or equal to 0" value. This
should resolve a flakey test and a seemingly erratic production error.
Resolves https://app.honeybadger.io/projects/66984/faults/83596547
* Favoring min over adding 1
Due to syncrhonization antics, let's not assume time is consistent
across servers.
Timey Whimey Wibbley Wobbley
* Update app/services/articles/feeds/weighted_query_strategy.rb
Co-authored-by: Daniel Uber <djuber@gmail.com>
Co-authored-by: Daniel Uber <djuber@gmail.com>
* 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.
We are loading the badge for each included tag in the
Search::TagSerializer and seeing warnings from bullet
```
Bullet::Notification::UnoptimizedQueryError:
GET /search/tags?name=ta
USE eager loading detected
Tag => [:badge]
Add to your query: .includes([:badge])
Call stack
/home/travis/build/forem/forem/app/serializers/search/tag_serializer.rb:5:in `block in <class:TagSerializer>'
/home/travis/build/forem/forem/app/services/search/tag.rb:11:in `serialize'
/home/travis/build/forem/forem/app/services/search/tag.rb:7:in `search_documents'
/home/travis/build/forem/forem/app/controllers/search_controller.rb:54:in `tags'
/home/travis/build/forem/forem/app/lib/middlewares/set_time_zone.rb:10:in `call'
```
Follow the advice, now when multiple tags are in the result set,
having multiple badges, badges are loaded only once (at query time)
and not one by one (at serialization time).
* Allow for skipping navigation link creation
This commit provides a possible solution for preventing the re-creation
of a navigation link deleted by a Forem creator.
What I need is a discussion around the life-cycle of the application
installation and updates.
In particular, does this provide a robust enough mechanism for resolving
the issue at hand?
_Note: it pains me to ask about a user's role, but this is provided as a
point of discussion and possible implementationi to address the
underlying issue. But I'm referencing a constant in the Rake task so
hopefully that will help future refactors. Also, it's one reason I
needed to remove the `private_constant` declaration._
If we accept this code change, a future task is to document the ENV and
behavior.
Closes#15960
**Further considerations**:
- How might we refine this to not be as "role" reliant?
- Could we have a Site::Setting that we enable/disable
regarding the navigation links?
Regarding QA:
- Start from an empty database
- Run setup
- Verify Navigation Link exists
- Delete Navigation Link
- Run setup again
- Verify Navigation Link exists (because we don't have a user)
- Run seeds
- Delete Navigation Link
- Run setup again
- Verify Navigation Link exists (because we don't have a user)
```shell
$ cd ./path/to/forem/repo
$ rails db:drop db:create db:schema:load
$ bin/setup
$ bin/rails runner "puts NavigationLink.where(url: '/readinglist').exists?"
=> true
$ bin/rails runner "NavigationLink.where(url: '/readinglist').delete_all"
$ bin/rails runner "puts NavigationLink.where(url: '/readinglist').exists?"
=> false
$ bin/setup
$ bin/rails runner "puts NavigationLink.where(url: '/readinglist').exists?"
=> true
$ bin/rails db:seed
$ bin/rails runner "NavigationLink.where(url: '/readinglist').delete_all"
$ bin/rails runner "puts NavigationLink.where(url: '/readinglist').exists?"
=> false
$ bin/setup
$ bin/rails runner "puts NavigationLink.where(url: '/readinglist').exists?"c
=> false
```
* Bump for travis
* 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>
* Restoring tracking of custom impressions without GA
In #15967 I (@jeremyf) introduced the "don't do tracking if Google
Analytics ID is blank". However, we still likely want to
trackCustomImpressions().
Further, I did some sleuthing and adding a couple of comments on whether
we should record page views for articles in draft status or any views by
the author.
* 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.
* Revert "Ensuring we don't track views of author or unpublished"
This reverts commit 321ecbed0ac4552e175d39bf17655a2fc93b265c.
In my quest to find `where(attribute: value)` in controllers and even
services, I stumbled upon this pattern.
As of this commit (and prior) an Article's associated user is it's author.
* Don't update social information for suspended/banished accounts
* Prevent suspended users/accounts from updating their profile information
* Add tests and fix some logic
* feat: hide the logo_svg behaind a featur flag if we've ennabled it
* feat: show the input field iis the feature flag is enabled
* feat: show the new logo when the feature fkag is enabled
* chore: change working
* feat: add a logo spec
* fix: with the updated changes we show a community name if there is no logo, hence we sometimes would need to update the community name instead of the logo on preview
* fix: use innerText
* Update app/javascript/admin/controllers/config_controller.js
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* empty commiit
* empty commiit
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Prior to this commit, I was seeing a "warning: nested repeat operator
'+' and '?' was replaced with '*' in regular expression"
```shell
> bundle exec rspec spec/liquid_tags/organization_tag_spec.rb
./app/liquid_tags/organization_tag.rb:5: warning: nested repeat operator '+' and '?' was replaced with '*' in regular expression
[Zonebie] Setting timezone: ZONEBIE_TZ="Dublin"
...
Finished in 0.39363 seconds (files took 2.89 seconds to load)
3 examples, 0 failures
```
After this change, when I run the same spec I get the following:
```shell
❯ bundle exec rspec spec/liquid_tags/organization_tag_spec.rb
[Zonebie] Setting timezone: ZONEBIE_TZ="Saskatchewan"
...
Finished in 0.39327 seconds (files took 3.03 seconds to load)
3 examples, 0 failures
```
The warning is gone.
Originally, I had changed `(?:[\w-]+)?` to `(?:[\w-]*)` which resolved
the warning.
But lookinig a bit further, we didn't need that
non-capturing group at all (`(?:[\w-]*)` is equivalent to `[\w-]*`). So
I further condensed this down by removing it. Likewise we my
understanding of regex is that `(?:/)?` is equivalent to `/?`.
So I stripped things further out.
I also noted a lingering issue to consider regarding what to do about
a URL that isn't an organization. This is a larger issue to consider as
we bring in Embed tags that are for Forem resources.
This change restores setting usernames to something less obnoxious.
Prior to this commit, I would on occassion get the following error in
seeds:
```shell
❯ bin/rails db:seed
Seeding with multiplication factor: 1
1. Creating Organizations.
2. Creating 10 Users.
rake aborted!
ActiveRecord::RecordInvalid: Validation failed: Username is too long (maximum is 30 characters)
./forem/db/seeds.rb:67:in `block (2 levels) in <main>'
./forem/db/seeds.rb:60:in `times'
./forem/db/seeds.rb:60:in `block in <main>'
./forem/app/lib/seeder.rb:30:in `create_if_none'
./forem/db/seeds.rb:57:in `<main>'
<internal:~/.rbenv/versions/3.0.2/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
<internal:~/.rbenv/versions/3.0.2/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
-e:1:in `<main>'
Tasks: TOP => db:seed
(See full trace by running task with --trace)
```
Related to work done in #16067
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates. By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).
By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values. As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.
This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).
_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else. I assume that
I'm going to break some cypress tests and will need some help fixing
them._
Closes#15916, #14704
Supersedes #15983
How to test locally:
Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch. Then in `rails console` find a user that's written articles:
```ruby
user = Article.last.user
user.update(name: "\\: #{user.name}")
user.articles.each(&:save)
```
Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).
Then get a logged in and a logged out browser session going. Open your
web inspector and open console. Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.
On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).
Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.
On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.
More importantly, the Follow buttons should work.
* updates
* updating doc
* Fixed broken documentation in Storybook stories.
* bring back a element
* md --> mdx
* c-* doc update
Co-authored-by: Nick Taylor <nick@dev.to>
* Add validations and constants for feed_style and strategy
* Convert text fields w/ static options to dropdowns
* Convert Bootstrap selectpicker to native HTML select tag
* Update test to use valid feed strategy
I was working on another branch and as part of my commit, Rubocop
removed a validation (but not the spec that asserted the validation).
Below is the "non-updating" rubocop offense on the other branch.
```shell
❯ rubocop ./app/models/notification_subscription.rb
Inspecting 1 file
C
Offenses:
app/models/notification_subscription.rb:13:29: C: [Correctable]
Rails/RedundantPresenceValidationOnBelongsTo: Remove explicit presence
validation for notifiable_id.
validates :notifiable_id, presence: true
^^^^^^^^^^^^^^
1 file inspected, 1 offense detected, 1 offense auto-correctable
```
To remediate, I ran:
```shell
> rubocop --only "Rails/RedundantPresenceValidationOnBelongsTo" \
--auto-correct
```
This resolved the `app/models`. Then did some regex magic and removed
the assertions from `spec/models`.
For Forem folks, I wrote a [forem.team post][1] discuss if this is how
we want to proceed.
[1]:https://forem.team/jeremy/rubocop-auto-updating-mayhem-33a6
* Adding a convenience/optimiization method.
Without this method, the `@object` will handle the `decorate` message;
which will go through the logic of determining the decorator class, and
isntantiating a new decorator.
Related to but orthogonal to #16078.
* Update app/decorators/application_decorator.rb
Co-authored-by: Jamie Gaskins <jamie@forem.com>
Co-authored-by: Jamie Gaskins <jamie@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
* Don't send auth broadcasts for providers that are in beta
Currently we have :apple as a restricted provider (you can enable it,
but it's treated as beta here, rather than generally available).
While we were correctly checking if you had all GA providers enabled
in authenticated_with_all_providers?, we were incorrectly pulling
all enabled provider names in find_auth_broadcast (the message to send
the user), and picking apple_connect.
Since it doesn't make sense to omit apple id login from consideration
when checking if all available auth methods are used, then recommend
that it be used consistently, capture this "GA" state as a method, and
use it both in the test "does this user have all available identity
providers enabled?" and the selection "which identity provider can I
suggest they setup?" consistently.
Since we're about to enable google as an auth source (in #15986) I'll
check with Josh if he expects this to be GA on release or in limited
beta.
* Clean up authenticated_with_all_providers?
We have a method identities that returns the enabled identities for
the user (a relation), and a method ga_providers that returns a list
of enabled and not beta provider symbols.
Change the set difference to use Array#all? (which will exit early on
the first failure). Efficiency note: while I think this reads
better,it's possible this issues a number of small (cheap) queries for
identity by user id and provider id, but there's a unique index on
(provider, user_id) that should be effective.
* Only check providers that have active broadcast messages
An admin can stop sending "connect using apple" follow ups by
disabling that broadcast.
I randomized the enabled/active broadcasts for connection options so
they're not always pulling the same (facebook? apple?) option every
time.
* Clean up lost thought in comment
* Moving the "Null" user object closer to User
Prior to this commit, we had the presentation concept of a DELETED_USER
in the ApplicationHelper. Further, we did type checks against that
object instead of relying attributes of the object.
With this commit, I moved the "Null" user closer to the User definition
to help highlight the concept that there might be deleted users.
I didn't remove all of the type checks, but did attempt to create a more
"duck-type" object.
Further, I moved away from an OpenStruct which in the past (and perhaps
present) had performance issues.
* Moving DeletedUser into Users module space
ActiveSupport adds DateAndTime::Calculations#after? (and before?) -
which clarifies intent (users newer than the relative time are
skipped) of the early returns.
