* Expect to raise error should use a block
This is kind of pedantic, since we _had_ been using a lambda, and
calling as a value. Now explicitly call the proc in a block and expect
the block to raise error.
* Replace lambda with block
Replace lambda passed as a value to an implicit block with a block.
* WIP - Conditional rendering of "Create Post" link
This PR builds on the conversation from forem/forem#17056 and moves in a
slightly different direction.
Important in all of this is that the ability to create a post is
enforced on the server. If the "Create Post" button were to be visible
but the user couldn't create a post when they clicked the button, they
would get an authorization error (or some such response).
This PR posits a different and perhaps competing approach to
forem/forem#16606. This PR provides a general approach in which we add
class attributes in our HTML erb files.
Note: I have not included Cypress tests as I don't want to yet commit
that time. I'm also wondering if this is the "right" thing to do. I
definitely think we want to add some JS tests. But could we do JS and
unit tests? (How do we reach consensus regarding our test approach?)
Again, thank you for the conversation and expect an even more "complete"
PR after we have our discussion.
* Extracting AsyncInfo model to ease testing
* Renaming forbidden to visible
* Adding cypress test to assert no 'Create Post'
These are always treacherous. What happens when we rename the button?
The test will continue to work.
* Apply suggestions from code review
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Switching to pack file for policy
Follows on [Suzanne's comment](https://github.com/forem/forem/pull/17076#issuecomment-1088567286)
* Update app/javascript/packs/applyApplicationPolicyToggles.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Prior to this commit
This would have been a bug, if someone had:
1. Toggled on the :limit_post_creation_to_admins feature flag
2. Wasn't an admin
3. Already had a published post
4. Went to edit the post and attempted to preview it
Related to the following issues:
- forem/forem#16908
- forem/forem#16944
- forem/forem#16837
- forem/forem#16942
* Disable current period specs
These are failing about half of the time (when the test runs east of
UTC, like Perth or Astana, the counts are incorrect, when the test
runs west of UTC like America/Chicago or Alaska, it passes).
* Cheat by setting clock to noon UTC
This aligns article published dates in ruby with those calculated by
postgresql. Copy the decision made in
https://github.com/forem/forem/pull/3518 for the analytics service.
* Remove testing comments
I had added titles to some of the articles I wanted excluded from the
result to confirm they were being included.
* transliterate when generating attribute names to avoid emptiness
If you enter a non-ascii (non `\w` matching) string as the profile
field label, the attribute name is the empty string.
This causes problems outlined in #16391
To avoid losing user provided data, transliterate (Sterile is the same
tool we're using for Article#title_to_slug) before matching against
the word regex.
If we persist an empty string, or persist a non-`\w` name, the
coordinating regex in Profile::ATTRIBUTE_NAME_REGEX will leave a nil
match and raise NoMethodError (the method missing is for
match[:attribute_name] when match was nil, not Profile - that's the
next commit.
* Guard against nil matches
If an attribute name doesn't match the regex, the match is nil, and
trying to access (nil)[:attribute_name] raises a NoMethodError.
If there was no match, assume profile does not respond to the
selector, and don't handle it in method missing.
* Ensure generated attribute name is valid before saving
This raises a validation error if the generated name (from the label)
would be empty.
It's not an optimal error message (since the user can't see the
internal attribute name) but it prevents persisting broken/empty data
* actually raise error when validating
validate/valid? only return true or false (and set errors on the
object). In order to reject the creation, we need to raise a
validation error, not only call validate. I think this is because the
execution of before_create hooks happens after validation (which is
why the validation was only checked on update, not create).
* Generate an attribute name completely independent of the field label
This prevents mistakenly labeling a field "Class" or "Association" or
one of the other hundred public methods an AR model like Profile
exposes. Since attribute_name will be passed to `profile.public_send`
we really shouldn't build selectors from user supplied inputs.
* Use the admin supplied label in the sidebar
The profile decorator is used in the Users#show page to populate the
sidebar fields. Don't use the attribute name (which we mangled during
creation, and now generate randomly) as the label, use the label.
* Make the label lookup null safe, and filter attributes more
* Update data update script to not expect predictable labels
This is low impact since it ran in july, but we no longer know what
attribute name a label will create.
* Fix moderator spec
"Test Field" label no longer predictably generates :test_field as an
attribute name. Ask the field what it's name is before asking profile
about it.
* Fix profile preview card request spec
Remove the assumption that profile responds to a method name based on
the label for Work and Education fields.
* Update old DUS and its test
This can probably be archived
* Update profile spec to use fields generated attribute names
We used to "know" how attributes were generated from labels. Now we don't.
* update e2e seeds for profile field change
* Don't expect attribute name to be based on the label
* expect created profile fields respond to their attribute name
* Update system test
The label, not the attribute name, is shown on the profile form (the
field has an id related to the attribute name, but the view shows the
mutable/human-readable label).
* Keep the field title lowercase when sending the json preview card
The userMetadata component expects "work" and "education" to be
attributes of the metadata, but the ui_attributes_for() method was
titlizing these (for display).
Ideally we wouldn't have "special purposed" these two field names, but
they're there.
* update profile field by attribute name, not based on label
* Update profile field removal assumptions
We don't know what the method selectors will be, we have to ask.
* remove old test
* Update translations for Education and Work
Since the ui_attributes_for(area:) now gives the label, not the
attribute, we need to match the label of the profile field.
Note to self: this exposes an issue in localizing the custom profile
fields (probably a bigger problem for large, international communities
like DEV than some others, but trying to match static translation
files against user-modifiable database records seems like a problem
we'll see again).
* Empty commit to retrigger buildkite
* Remove profile field migration update scripts
Cloned the specs from the other "remove unused scripts" script.
* Remove unused scripts
The data update script removes the entry from the table (recording
that these have run) - we also want to remove the files (preventing
them from running again).
* remove unneeded spec for removed file
* when translation for header area field not found, use the title
Only Work and Education already have keys in the yml translation file,
and there's not a great (or easy?) way to make multiple translations
on these fields right now.
Since an admin can create a new field, and assign it to the header
area, we can't assume the code has a configured translation key for
this field.
Fallback to the title (we do this in another context already) if
there's no translation.
* PR feedback: Avoid n+1 query for labels
the original implementation of "label_for_attribute" had an n+1 query
looping over each matched key.
Follow suggested improvement and pull labels and attributes at once
from the db and modify the returned hash.
* Downcase title before looking for translation key
This avoids putting "odd" capitalized keys into the yml translation
file
Revert addition of "Work" and "Education" to the users files.
* use a let binding for duplicated test data
* Update app/models/profile_field.rb
prefer SecureRandom.hex for a dashless uuid (instead of removing the dashes).
Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
There are three major things occurring in this pull request:
1. Renaming `Article#update_cached_user` to `Article#set_cached_entities`.
2. Reducing an organization's direct knowledge of which of the org's
attributes an article caches.
3. Removing duplicate calls to update the article associated with the
organization.
For renaming to `Article#set_cached_entities`, the prior method implied
we were updating the persistence layer. However, we were not making any
save nor update calls. This rename should clarify intention.
For reducing knowledge, the comments for
`Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY` should explain the details.
And last, removing the duplicate calls; we had three methods that were
attempting to build and update the `Article#cached_organization`'s
value.
Closesforem/forem#17041
* Added a way to sort comments on an article
* Sorting of comments now uses crayons-dropwn. Some minor fixes as per comments on PR
* Add and fix test cases for sorting comments functionality
* Changes in code for sort comments. Code is more aligned with forem's conventions
* Added cyperss tests for sort comments on an article. Cleaned up code to better follow forem conventions
* Get fresh handle of triggerButton everytime clickOustideListener is clicked. Fix Cypress test cases to reflect the earlier.
Prior to this commit, we determined the 404 status based on the final
scoped query of the articles. That scoped query would limit to a
timeframe.
However, what we want is to not render a 404 status if we have an
"established" tag. An established tag means that we have at least one
published article (regardless of when we published).
Fixesforem/forem#16932
* Award badges for github commits with various milestones
* Fixes on award badges to github commits
* Add data update script to add badges
* Disable award_multi_commit_contributors
* Revert "Disable award_multi_commit_contributors"
This reverts commit 1852f34fd45bd23b716cf80ebbd9ce2c1879a819.
* Remove DUS
* Change badge name
* Only use award_contributors
* Simplify specs
* Revert "Simplify specs"
This reverts commit 230514ccea68445b057991d3d536fbb2a590a3b1.
* Revert "Only use award_contributors"
This reverts commit a92735a079f43610aedc805f74f032466d23f1cf.
Co-authored-by: Mac Siri <krairit.siri@gmail.com>
There's an existing data update to introduce the connect feature flag,
and it's been disabled. However, since all the code relying on this is
now gone, it makes sense to remove the flag.
* Fixed hiding 'Click to edit' link for drafts
* Improve "click to edit" link spec
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Make the expectation more clear
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Trigger Build
* WIP - use turbo to render html for create post button
todos and discussion topics
[] add tests (probably gonna be a lot of cypress 😅😭)
[] nominclature & file structure for authorization controller/routes
[] is this "authorizations" namespace approach the best?
[] authorization strategy relies on current user and caching doesn't
like `current_user`
* Use URL helper and not change the policy just yet
* put the turbo frame behind a feature flag
* Trigger Build
* Use URL helper and not change the policy just yet
* :)
* Add some test coverage and use same feature flag for consistency
* rubocop
* more rubocop
* trigger build
* adding a small sleep to see if specs pass 😩
* remove sleep
* disable Turbo session drive
* WIP
* Revert "WIP"
This reverts commit ce838672069fd703122156a0d82e5e6ad5398e0a.
* Moving spec from dashboard to root
With forem/forem#16913 the underlying test broke because we were
redirecting on the dashboard.
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
* Avoid using robohash for article cover images
If their service slows down, loading the cover image times out in
system tests.
Use a locally served image file instead.
* Specify expected image format for view object test
We were relying on the internals of Faker to return a robohash.org url
and checking that we included that in the cloudinary url.
Rather than relying on the default behavior, explicitly pass a
robohash url in the main image for this spec.
Prior to this commit the following situation existed:
> The path /dashboard/analytics/org/:id requires user
> authentication (e.g. signed in). However, it does not enforce
> authorization. Anyone can see this page. The page, however, uses
> javascript to populate the data. So no information, aside from the org
> name associated with the :id leaks out. The javascript API end point
> enforces organization membership.
>
> I would expect that the authorization in the HTML rendering would be
> the same as the javascript API end point.
This commit ensures that the dashboards#analytics end point uses the
same policy logic as the API analytics end points. Further, it keeps
folks who aren't org members out of the base HTML page for other orgs.
Closes forem/forem/#16985
This commit provides two things:
1. Some notes related to my analysis regarding the dashboard
2. Conditional redirects and rendering based on article policies
The code comments say most of what I want to say, but to reiterate:
When a user can't create articles nor do they already have published
articles, then we don't want to avoid showing them stats related to
articles.
Closesforem/forem#16913
Related to forem/forem#16908 and forem/forem#16931
* Removing a JS message about connect
Related to forem/forem#14734
* Remvoing another reference
I ran `rg Connect[^i]` to see about any remaining references to
Connect. This one seems to be the last.
This menu item will show up once you have added the
`:limit_post_creation_to_admins` feature flag (regardless of whether
it is enabled or not).
If you wish to add the menu item:
```console
rails console runner "FeatureFlag.add(:limit_post_creation_to_admins)"
```
In "adding" the item, it will default to disabled. However, as an
administrator you should now be able to toggle on and off the
authorization enforcement.
If you wish to remove the menu item:
```console
rails console runner "FeatureFlag.remove(:limit_post_creation_to_admins)"
```
Future plans for this will be to remove the `FeatureFlag.exist?`
conditional so that the menu item shows up. A major consideration is
that we'll assume that all Forem's have a "default space" in which
folks (by default) can post.
Builds on forem/forem#16897Closesforem/forem#16842
* Adding ArticlePolicy#has_existing_articles_or_can_create_new_ones?
As part of our aspirations to only show users what is relevant to them
and "hiding" what is not, this method will help us with the edge case of
"should we show the user a dashboard listing of posts?"
Related to forem/forem#16837
* Adding further documentation
* Adding clarifying comment
* Always show the browse section regardless of featured
* Add tests for /pod
* Use safe operator if there are no episodes to show
* Fix test for new podcast page
* Fix test again for podcast page
* prefer case to multiple if branches
case klass works fine on inheritance chains (we don't need to match on
inheritance explicitly).
* Verify case on error class behaves as before
* Fix tests
I don't know how I committed tests that were failing, but don't do that
As I was looking to implement the new Spaces feature (see
forem/forem#16842), I began looking at how to adjust the admin menu. I
found the Menu and AdminMenu. I wanted to add the "spaces" item under
the `:content_manager` scope. Looking at the implementation, I saw I
would need some additional branching logic.
To get an understanding of the implementation, I chose factor towards
classes.
This refactor does a few things:
1) Clarifies a method name (e.g. "children?" becomes
"has_multiple_children?")
2) Adds some tests of the menu implementation.
3) Removes deeply nested hashs in favor of first class objects.
4) Removes a complex method for determining menu visibility, instead
favoring a method that either takes a boolean OR a lambda.
There are further refactors to consider, especially in regards to the
helper methods and some of the coercion of strings into different
formats. But this refactor gets me the part I most am interested in:
Making it easier to specify a menu item as visible or not.
* Penciling in a Default Spaces section
This delivers two primary things:
1. Extracting shared policy examples
2. Providing a functioning skeleton for toggling on the
"limit_post_creation_to_admins" feature.
Important is that once merged, it would be possible for this code to
"leak" out. But how that leaks out is also how you can test that the
feature works.
First, to orient, there is a constraint for a new route. That
constraint checks if the feature flag exists (e.g. has been explicitly
enabled or explicitly disabled). In other words, at this point, you
can't accidentally enable the feature via the UI. But once you have
enabled the feature, you can then toggle the feature on and off.
So, to test this in the browser:
1. Pull down this branch
2. Run `rails runner "FeatureFlag.disable(:limit_post_creation_to_admins)"`
3. Startup your the web server.
4. Login as an administrator
5. Goto /admin/content_manager/spaces
a. Bask in the glory of a plain HTML form
6. With another browser, login as another non-admin user
7. Go to /settings/extensions, you should see a section Publishing from RSS.
8. Now with the admin's session, update the form to turn on
"limit_post_creation_to_admins"
9. Back to the non-admin browser, refresh /settings/extensions, you
should no longer see the section Publishing from RSS.
Note: I have not included an admin menu item as that is related to and
dependent on some refactors I'm working on (see forem/forem#16888 and
forem/forem#16847). So a bit of "security through obsurity"
Note: In the future, once we resolveforem/forem#16490, we'll start
toggling the "Create a Post" button.
Note: I am not including Cypress tests nor request tests for this
feature because the implementation details related to the testing via
that approach are a little too volitale.
Related to forem/forem#16842
* Updating copy based on forem/forem#16893
* styles
* styles
* dark styles
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* complete IG implementation of profile embed, refactors, update specs
* Follow redirects and check them for validity via recursion
* working out redirects
* add IG test links with params
Co-authored-by: Dwight Scott <dwight@forem.com>
* Adding docs and specs to AdminMenu.nested_menu_items
This relates to PR forem/forem#16847 which addresses issue
forem/forem#16842.
The goal of this PR is to help me develop an understanding of the
AdminMenu so I can further extend it and better understand it.
There's also a bit of knowledge sharing that I'm looking for, and the
comments and tests are there to help "confirm" that knowledge. There
might be more that I'd consider regarding a refactor, but I want to make
sure I'm on the right path before I go further.
tl;dr - this is the smallest commit I can make to begin to tease out an
understanding of the method I put under test.
* Update app/models/admin_menu.rb
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
* Adding more documentation
* Update app/models/admin_menu.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* feat: add the feature flag route and controller
* feat: add a ff page
* feat: update to a post with a success banner
* feat: test the feature
* feat: update the styling
* feat: change feature flag to extension witha few more improvements like adding a model not backed by a table
* feat: update the text
* refactor: if you enable a FF, it gets added under the hood as well
* feat: accessibility name for label
* use keyword args
* use locales
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/extensions/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update config/locales/controllers/admin/fr.yml
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/controllers/admin/extensions_controller.rb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* fix: rubucop
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Removes admin_member_view feature flags and refactors conditionals
* Removes unused admin/users/_email_tools partial and user_email_tools_spec
* Actually remove the admin/user_email_tools_spec this time
* Removes the edit route from admin.rb users
* Adds a DUS (and spec) to disable and remove admin_member_view flag
* Fixes admin/users_spec.rb failures by updating spec with Member Detail changes
* Fixes spec failures within admin_bans_or_warns_user_spec.rb
* Refactors Admin::UsersController#credit_params due to failing specs
* Removes last traces of edit_admin_user_path and fixes buttons, specs, etc.
* solution
* fixing specs
* complete specs
* nudge Travis
* resolve database issues
* nudge Travis
* Check that the settings table exists before loading the class
This maybe fixes an issue in test-console-check accessing a missing
table `users` (because we're requiring
settings during the initializer, and we're initializing the app during
a schema load on a newly created db).
* refactor check
* privatize and rename
* add self
* reposition private class method
* Prevent accidentally deploying to DEV
* Skip tests to try deploying to benhalpern
* Allow PR to deploy to benhalpern
* Allow other branches to deploy
* Oops
* Revert changes to travis.yml
Co-authored-by: Dan Uber <dan@forem.com>
Co-authored-by: Andy Z <17884966+Zhao-Andy@users.noreply.github.com>
* Normalize article titles
* Change method name to match what it does
* Expand on allowed emoji ranges
This required a few changes to be in compliance with Rubocop because
that regex is a beast.
* Limit feeds import fanout to users with feeds
This filtering for feed settings occurs in
Feeds::Import#filter_users_from already, so the enqueued ForUser
jobs were mostly no-op, but the queue latency spiked as hundreds of
thousands of jobs were added to default.
Prescreen users to avoid enqueuing a noop job every hour for every single
user without a feed.
* Update tests to check only jobs for users with feeds are enqueued
* spec cleanup
Only include alice when we check no job is enqueued for her.
Assert no job is enqueued for alice.
Rename bob to user when alice isnt there to contrast.
* Update spec
Use update_columns to bypass setting validation (checking feed is
valid/reachable)
Remove unneeded timecop block when passing a set time
Remove sidekiq: fake and allow Import.call to be received
* Empty commit to trigger CI rebuild
We had been (since the beginning) disabling webmock in this
test (which means vcr was _not_ intercepting requests and we were
making them live). This appears to have been a work-around for an
issue where the captured URL in the cassette pointed to cloudinary,
while the code was fetching from the source directly, and an unhandled
request error was raised when running the spec.
However, the upstream source (https://via.placeholder.com/350x150)
activated cloudflare bot detection, and rather than a png image
response, we were getting an unprocessable html gateway page "Checking
your browser..." from cloudflare, and the test was failing.
Point the vcr cassette at the same url the test checks, and remove the
local webmock disable.
