* Apply overflow-wrap fallback everywhere we use anywhere value
* fix some more non breaking spaces layout issues
* user with org sidebar
* comment index header
We're seeing these fail intermittently (typing the color doesn't
result in the desired color in the form), and will disable temporarily
to avoid unrelated build failures.
This commit does three things:
1) Adds and tests the ArticlePolicy#moderate? feature
2) Refactors the spec helpers to be a bit more flexible
3) Adds a few more edge case tests around ArticlePolicy
My apologies for conflating these changes, as it makes this commit
more challenging to review.
The goal is to adequate demonstrate the logic around users who can or
cannot moderate. At present, the rules for who can moderate is here:
841491c6ee/app/javascript/packs/articleModerationTools.js (L17-L27)
```js
if (user?.trusted) {
if (user?.id !== articleAuthorId && !isModerationPage()) {
initializeActionsPanel(user, path);
initializeFlagUserModal(articleAuthorId);
// "/mod" page
} else if (isModerationPage()) {
initializeActionsPanel(user, path);
initializeFlagUserModal(articleAuthorId);
}
}
```
- Related to forem/forem#16783
- Closesforem/forem#16784
Videos are a conceptual subset of Articles (see
[VideosController#create][1] for supporting "evidence"). As such, we
want to ensure they conform to the expectations of the ArticlePolicy (as
described in forem/forem#16483).
This feature enhancement does a few things:
1. Renames swaps the `VideoPolicy#new?` and `VideoPolicy#create?`
declarations (reversing the alias direction)
2. Moves methods out of the concrete ArticlesPolicy into the
ApplicationPolicy.
3. Exposes these user oriented methods for external
consumers (e.g. making future refactoring work easier). The happy
benefit is that this already removes a bit of duplicated logic.
4. Highlights that while yes the Policy is correct, we also may be
flirting with the concept of some sort of User "properties as it relates
to authorization" logic (good standing users, established users,
etc.)
5. Reworked the timing of a guard clause and setting of instance variables.
[1]:
33e9bac0f7/app/controllers/videos_controller.rb (L18-L22))
Relates to #16483Closesforem/forem#16728
To test this, I'm relying on the existing test suite. I envision that
shifting from returning false for suspended users to raising an
exception might cause some false positive errors. Note, in the
[AppliciationController][2] we handle the responses. Paired with
[config/appliciation.rb][3], we handle the policy exceptions.
[2]:33e9bac0f7/app/controllers/application_controller.rb (L32)
[3]:33e9bac0f7/config/application.rb (L71-L81)
With this commit, we're only allowing users who can create articles to
enter RSS feed information for fetching of articles.
Note: I chose not to indent body of the if conditional to ease the code
review. I also chose to place the if statement in this partial instead
of in the [app/views/users/_extensions.html.erb][1] file; this helps
contain the logic around the policy.
For sleuthing this mirrors the approach of forem/forem#16735Closesforem/forem#16788
Related to forem/forem#16766, forem/forem#16732, and forem/forem#16763
[1]:b87fd77992/app/views/users/_extensions.html.erb (L3)
In looking at forem/forem#16787, it felt like we would benefit from a
similar approach as we adopted for nullifying blank strings.
This refactor helps pave the way for a "normalize_text_for" method (or
some such thing; naming things is hard).
This change also involved introducing a `describe` block into the
associated spec.
* Fix broken documentation link
When sending an invitation, and smtp isn't configured, we show a link
to the old smtp-settings page, which has been renamed
email-server-settings in the admin documentation site.
* Remove explicit link target, assert link text is present
This spec failed when I changed the link target to match the
documentation. Rather than moving the target, I'm just asserting we
show a link to the docs (not where the docs are located).
* Only fetch RSS of users who can create articles
This commit leverages changes from forem/forem#16732 and
forem/forem#16763 to ensure that we're only fetching RSS feeds for
folks who have permission to create articles.
In addition, I chose to rename the variable `user_scope` to
`users_scope` to bring consistency in the variable names.
I also chose to refactor a private method so that it wasn't setting two
instance variables but was instead returning a value that could be used
to set an instance variable.
Closesforem/forem#16487
* Amending method name
Note, I'm not overly keen on writing permission tests for this
component. Why? Robust permissioning tests can create combinatorial
explosions. And there are presently no Rspec request specs for this.
So to add an automated test, we'd need to add a set of seed data that
seeds data that conforms to the emerging business logic of the policy.
And while this is easy with use case 1-1, it gets harder as we move
into more nuanced use cases. Instead we should rely on bombarding our
policy classes with lots of tests to let them demonstrate what we mean
when we say `if p.olicy(Article).create?`
Note, there is a potential relation to forem/forem#14807, namely if we
add a rich text editor to our comments, we may need to explore the
purpose and intention of this setting.
Closesforem/forem#16516
While in essence an idempotent change the purpose of this PR is to also
favor explicit method calls over callbacks. Note, we don't have an
associated model (e.g. Video) and instead rely on `:video` which Pundit
converts to VideoPolicy.
From my experience in rails it can become challenging to mentally parse
the various before/after action paired with the only and except. As
well as considering the timing of what happens when.
Further this commit helps provide a record of "work" towards an issue.
Closesforem/forem#16729
Reverses forem/forem#3806
This change makes it easier to resolveforem/forem#16487.
What do I mean by that? To address forem/forem#16487, I need to only
select user's who are authorized to create articles. In
forem/forem#16732 I added a method that will allow chaining of a User
scope. So with this current commit and forem/forem#16732, I'm
triangulating on an approach that will make that change easier.
I did not want to conflate those two, as mixing this PR's change and
what is necessary for the closing PR would create a more complicated
review. Not unduly complicated, but one that will require more tests
and a change in logic. And for someone reviewing the diff, those
concerns could easily be lost.
Yes, I have changed the method signature, but that method signature is
limited to one location:
```shell
❯ rg "Feeds::Import.call"
app/workers/feeds/import_articles_worker.rb
21: ::Feeds::Import.call(users_scope: users_scope, earlier_than: earlier_than)
```
So I look to the method signature a bit as an internal API, hence the change.
Related to forem/forem#16487
* Use correct table heading for username
* WIP add role filter and crayonsify forms
* styling
* responsive
* Add form back in that was removed by merge conflict fix
* Suggest only valid search options
* Add basic tests for search and filter by role
* Add aria for search field
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Add email to aria
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
As I'm looking at limiting feed fetching only for users who are able to
create articles, I needed a means of querying "Who are the users who can
create articles?" This bit of work was the smallest chunk that I could
think of.
These kinds of questsions are going to propogate (e.g. give me a list of
all the people who can comment? who can create articles in this space?)
For now, this pattern should suffice.
Yes, there's duplication of knowledge between the policy's instance
method and class method. And there's a bit of knowledge bleed between a
user's method (e.g. `user.any_admin?` and
`*Authorizer::RoleBasedQueries::ANY_ADMIN`) but that's a small "evil".
Related to #16486
* when there is no div for it, don't render CommentSubscription
* Retrigger CI
* Retrigger CI
* don't write an error to the comment div if its gone
This fixes an issue seen in tests, where the page transitioned while
we were running the articlePage async handler, so the article-body
element was gone (can't access dataset of null), and the error handler
catch block tried to access the innerHTML of another getElementById.
* Use existing found element `root` when setting the error message
No need to find a new comment-subscription div to write the error to,
we already named the one we're interested in at the beginning of the function
* Remove dots from chart when time range is Infinity
* refactor: 🐛 Remove dots using the chart options
* refactor: ♻️ Improve params of drawChart func
* refactor: ♻️ Use a variable showPoints
* only init mod tools if we're still on article page
* Update app/javascript/packs/articleModerationTools.js
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
This commit compresses the policy check that spanned both :new? and
:enabled?. We hide the ability to upload a video (as per the altered
html.erb files of this commit) but we didn't enforce the same logic in
the controller.
Further, as Videos are a subset of Article, I'm adding the "verify the
user is not suspended" test.
Directly relates to #16483
For sleuthing this relates:
- #16728
- forem/forem#16537
- #16634
And for historical context, this relates to:
- #10955
- #10954
- forem/internalEngineering#149
* Adding indirection for Tag#name to ease future work
In pairing with Arit, we were able to capture a few small wins in
regards to favoring an accessible tag name. In other words, where ever
you see a tag's name, we want to render it as camel-case. This will
greatly help those using a screen reader and frankly visually scanning
the tag (idontknowaboutyoubutthisiskindahardtoread).
However, as we explored the
code base, we found several places that will require further
consideration. These revolve around the API and caching of tags for
articles and listings as well as the JSON documents we use for
populating drop-down lists.
So, instead of unleashing a large pull request, we're opting to provide
a small non-breaking refactor that demonstrates where we're going and
keeps production working as expected while allowing future
development/testing to benefit from these captured gains.
Our next steps are to revisit the related issue and do a proper task
breakdown, becuase there are too many considerations to call this a
"small win".
I also encourage reviewers to read the comments. This is an emerging
mental pattern that I believe helps us conceptually move our codebase
forward while also guarding against the mega-PR with oodles of commits
and file changes that span numerous contexts.
Related to forem/forem-internal-eng#269
* Expanding specs to address feedback
* Updates the copy for all modals within Admin Member Detail view
* Resolve most failing tests within managaUserOptions.spec.js
* Update app/views/admin/users/show/profile/actions/_export.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/seededFlows/adminFlows/users/manageUserOptions.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
