When we merge this, Forem admins will see the "Admin > Content Manager >
Spaces" section in their Admin area. This will then allow them to
toggle on and off spaces.
Closesforem/forem#17161
* Don't create podcast episodes without a media_url
There is an early return from add_media_url to prevent using an empty
enclosure_url, which avoids errors in
`GetMediaUrl.call(a_blank_value)`, however there's also a not null
constraint on podcast_episodes.media_url in the database, which causes
upsert to raise an error.
Returning early here avoids the error (which had halted the worker).
See also
https://app.honeybadger.io/projects/66984/faults/80833402
This error happens regularly, 96 times per day in DEV, it looks like at least
one podcast feed_url is for an rss feed filtered by /tag/podcast/,
(and at least one post was tagged "podcast" but didn't include any
media to get).
My understanding is at worst we'll ignore the error silently.
* Add test cases
given a blog post (not a podcast episode, no media url), don't raise
any error, don't create any episode.
Included codepunk.io feed that was showing this issue initially.
* Prefer compact_blank! to delete_if blank
Rubocop suggested this. Why would I argue.
* When sort_direction is anything other than asc or desc, remove it
There is initializer code in the search/query classes that handles nil
sort_direction by adding a default value, so deleting the sort
direction is reasonable here.
* Set a default sort direction for articles of :desc
* Only sort if sort_direction and sort_by are present
* Add test case for article search with invalid parameter
You had a good ride, but your successor, the
`Articles::Feeds::VariantQuery`, is doing well in production. And has
been for quite awhile.
You helped us get to a better spot in regards to the feed algorithm, but
your time has come to rest as a memory in our git history.
* Rename unsubscribe to remove_from_mailchimp_newsletters
* Permanently delete from mailchimp instead of unsubscribing
* Rename method and remove comment
* Don't use i18n because of error message
We don't want to use i18n here because e.title comes from the error response. If a Forem
was using a different locale and Mailchimp for newsletters, they would still receive the
error response in English (because of Mailchimp).
* Refactor and rename a bit
* Rename methods in tests accordingly
* Remove now moot test
* Rename method in other places it was previously called
* Remove unused translation, see 62fee247ee2565e4d3d14919a8636a76ce5ecc9f
* Fix typo and report error if not 404
As I was looking into the implementation details of the `/t/:tag_name`
relevancy feed I noticed an instance variable that we do not need.
Below are the results of looking for the instance_variable
`@article_index` or a potential `article_index` local variable or method name.
```shell
❯ rg "@?article_index"
app/controllers/stories_controller.rb
28: @article_index = true
131: @article_index = true
162: @organization_article_index = true
app/controllers/stories/tagged_articles_controller.rb
18: @article_index = true
app/controllers/stories/articles_search_controller.rb
7: @article_index = true
app/views/articles/_single_story.html.erb
26: <% if story.cached_organization && !@organization_article_index %>
32: <a href="/<%= story.cached_user.username %>" class="crayons-avatar <% if story.cached_organization && !@organization_article_index %> crayons-avatar--s absolute -right-2 -bottom-2 border-solid border-2 border-base-inverted <% else %> crayons-avatar--l <% end %> ">
79: <% if story.cached_organization && !@organization_article_index %>
```
* feat: remove feature flag on the member index view and re-arrange some partials
* feat: remove Feature Flag for the invitations view
* feat: remove Feature Flag for the gdpr view
* feat: remove Feature Flag for the controls
* feat: remove the feature flag in the tests
* remove extra queries that we no longer use
* Update app/views/admin/invitations/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/invitations/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/users/index/_controls.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/users/gdpr_delete_requests/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/seededFlows/adminFlows/users/userIndexView.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/seededFlows/adminFlows/users/userIndexView.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/users/gdpr_delete_requests/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* feat: attempt to fix Cypress test
* fix: soem other cypress tests
* fix: search bar
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
This commit does three things:
1. Breaks the inheritance of the VariantQuery.
2. Revisits the in-line documentation to better reflect current state.
3. Removes branching logic that we don't need.
My goal is to have a VariantQuery that is what we need, conforms to the
method interface of WeightedQueryStrategy but does not rely on it.
After we merge this, we can observe if things break on DEV and fallback
to WeightedQueryStrategy if this doesn't work. Once the
`Articles::Feeds::VariantQuery` has fledged on it's own, we can remove
the feature flag and then finally remove the weighted query strategy.
But that's the future steps.
Related to forem/forem#17393
* Update sort to have nulls at last
* Updating Article.sorting by published to only include published
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Similar to checking that "Unpin post" is shown after pinning, wait for
"Log in" to show after signing out before checking that signed out
users can see a pinned post.
Recommended by @aitchiss in a comment on #17385 as a way to limit
false negatives (where we check the previously signed in user's view)
* Allowing VariantQuery for Feed Generation
Apologies for the breadth of this pull request, I had considered many
small commits, but felt that would've been more effort for the value
provided.
This commit includes the following:
- Documentation updates to the feed variant (though not the final pass)
- Renaming and adding RelevancyLevers that help differentiate
- Adding RelevancyLever#range to provide documentation
- Reducing redundent controller logic by making a
`Articles::Feeds.feed_for` method.
- Adding some configuration validation for RelevancyLevers
- Adding constants for better clarification
- Testing unhappy paths for feed configuration
- Adjusting the module namespace of some objects
- Exposing top-level configurations for variants (along with their
defaults)
- Creating the VariantyQuery that at present inherits from the
`Articles::Feeds::WeightedQueryStrategy`
As implemented, we can deploy this code to production without using the
new VariantQuery. Once we toggle on the
`:feed_uses_variant_query_feature` FeatureFlag, it will switch to using
the VariantQuery. The VariantQuery's two variants and the
internal configuration of `Articles::Feeds::WeightedQueryStrategy`
produce the same query.
The goal of this factor is to allow for a quick on and off toggle of the
feed query; to ensure that what we introduce remains performant.
- Closesforem/forem#17272
- Closesforem/forem#17276
- Closesforem/forem#17216
In addition, I will be recording a code-walkthrough and linking that
recording to the pull request.
* Apply suggestions from code review
Co-authored-by: Mac Siri <krairit.siri@gmail.com>
Co-authored-by: Mac Siri <krairit.siri@gmail.com>
* Reload the config page after changing auth
When testing in Chrome (but not Firefox or Electron) I noticed this
test would fail. The issue appears to have been findByLabelText (used
to check Email enabled, Facebook enabled, etc are not visible) was not
finding the labels when the group was still hidden.
I _think_ there was a time, when this test was first written, that
submitting the form reloaded the page (simple form post), but that may
have been replaced with an xhr submission and some client side
replacement?
I tried not clicking the Authentication after enabling invite only (to
prevent hiding the form content, leaving this section expanded),
however this caused the "Email enabled" check to fail (it was still
visible). I think this means it had been passing because it was not
visible because the section was hidden, not because the label had been
updated properly after the submission.
I'm not sure if fixing the test here papers over a UI issue, but it
would be best if these tests worked consistently across browsers. I
suspect this may have behaved differently in earlier versions of
Chrome, or only been tested interactively in Chrome when the page
still reloaded, and this caused some drift.
* Update cypress/integration/seededFlows/adminFlows/config/authenticationSection.spec.js
prefer reload to calling visit a second time
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
We had seen an increase in CI timeouts (no output received for 10
minutes).
Running this example (signed out user sees the pinned article)
repeatedly I was able to reproduce the failure seen in Travis, and
checking the last few times this occurred it appears to be linked to
the pinArticle.spec.js last test, which fails with an
ApplicationPolicy when pinning.
The likeliest source of this issue is the POST (pin the article) is
received concurrently with the session delete (logging the admin user
out), resulting in a race. If/when the signout occurs before the
article policy is checked (does the current user have permission to
pin an article), the cypress running stalls.
This occurred both interactively (bin/e2e or cypress open) and
headless (cypress run/CI). The error went away reliably after
inserting this guard to check for the "Unpin post" link on the page,
which forces the POST to have completed and redirected back to the
admin page before logging out.
My understanding of this problem is borne out in the local test logs,
where the signout and pin requests arrive concurrently and process out
of order (and the POST gives a 404 instead of a 302).
Started DELETE "/users/sign_out" for 127.0.0.1 at 2022-04-20 12:09:23 -0500
Started POST "/admin/content_manager/articles/1/pin" for 127.0.0.1 at 2022-04-20 12:09:23 -0500
Completed 204 No Content in 42ms (ActiveRecord: 6.2ms | Allocations: 6323)
Completed 404 Not Found in 4ms (ActiveRecord: 0.4ms | Allocations: 833)
We avoid busting a user cache for 15 minutes but the tag cache is 5
hours. So we want to "nudge things along".
What this is trying to solve is the server side rendering of whether we
hide or show a button.
Related to forem/forem#17324
Related to forem/forem#17119
* Enable string uuid as namespace
This addresses a noisy deprecation warning showing in specs (it points
to our re-implementation of warden's sign_in_as helper, but I suspect
the issue is within warden since our initializer seems to match the
implementation in the upstream gem).
My understanding of this is that the prior default had been to pass
the provided uuid namespace as-is if it didn't match one of a few
named constants. New behavior is to validate that any string provided
as a namespace looks like a uuid representation or fail).
* Move configuration change to framework defaults
