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
**tl;dr** This PR is looking to provide a means to programmatically
assemble, from system configurations, what is currently the
[Articles::Feeds::WeightedQueryStrategy::SCORING_METHOD_CONFIGURATIONS][1].
Once we merge this PR, instead of having that constant, we'll initialize
the Articles::Feeds::WeightedQueryStrategy with the variant
configuration that we've assembled.
**Introduction**
This pull request has quite a bit going on, but as of now the production
code does not use any of it.
*Note: None of this code is bleeding into production code paths.*
Put your Ruby hat on and let's go for a ride. And apologizes for not
making this a smaller pull request. As I built this, I made many small
commits, but this became the smallest commit that provided the most
context without integrating into production code.
Before we get started, you may want to familiarize yourself with where
we're going. The `./app/models/articles/feeds/README.md` provides
further guidance; but fair warning reader, I have not reconciled the
README's language with what emerged as I wrote this pull request.
Those of you who are part of Forem Core team, you can read the
[Refinements to Our Present Feed Configuration][2]
to provide some insight into what's happening.
**Why not reconcile?** *Because I want to have all of the language and
thoughts available for you to help consider how best to name and model
this.*
**On to the Review**
The purpose of this pull request is to provide a mechanism for
engineering to provide a series of variant query levers. And to allow
us to easily configure those available query levers into a variant
query.
*Why the mix of Ruby configuration and JSON?*
The Ruby levers are meant to indicate that this is code we don't want to
expose in text based configuration because it might create SQL inject
points. (More on that when we integrate the code of this PR into the
production implementation)
The JSON levers are meant to indicate that "anyone" can pick the
appropriate/available levers and configure how they are set. In other
words, these values do not create SQL injection issues.
**What I Need from You**
I am putting this forward as a draft so you can read this code ahead of
time. I'm then going to schedule a synchronous code review where we
record our collective walk through of the implementation.
Closes:
- forem/forem#17268
- forem/forem#17269
- forem/forem#17270
- forem/forem#17271
- forem/forem#17273
Relates to:
- forem/forem#17308
- forem/forem#17245
[1]:6818ef3ed0/app/services/articles/feeds/weighted_query_strategy.rb (L84-L231)
[2]:https://forem.team/jeremy/refinements-to-our-present-feed-configuration-1p0c
From Anuj:
> Going forward we will be using heavy font-weight for the titles of all
> the admin sections. So it’s good consistency-wise.
This resolves that issue.
Closesforem/forem#17327
This div tag is subsumed by the line below (e.g. `<%=
application_policy_content_tag("div"...`). When the unclosed div is
present there is some undesirable layout offset. When it's removed,
things line up a bit nicer.
Discovered while attempting to reproduce forem/forem#17324
This commit modifies two things:
1. The sort order of the feed.
2. Locking in the next incumbent for the feed.
In consultation with Jamie regarding the SQL, I've switched from a WHERE
IN type clause to creating a JOIN. This allows for the calculated
relevancy score to become a sortable value for the feed.
Second the incumbent feed is a slight modification of the past winner;
based on the results of a long running experiment. This adjustment is
per conversations with product.
The past experiment ran since early February, so it's time to retire it,
regroup and move forward.
Closesforem/forem#17307
* add expandable search control at mobile size
* add invitations smaller layout
* replace missing member image
* tweaks following merge
* minor layout tweaks
* add actions to smaller view, update cypress specs
* ContextNotification model
* Create and destroy context notifications when notifications are sent/unsent
* Added a transaction when creating notifications and context notifications
* Documentation for context notifications
* Update article builder spec
In #16536 the tests were updated to match a change in the
logic (rather than "needs authorization" the response was "store
location", and we checked for those values).
In #17294 we re-adjusted the meaning of the second returned value from
Articles::Builder.call, and the call sites now label the second value
as "needs_authorization" again.
Restore the same name to the call sites in the spec, and simplify the
example descriptions (this might be a mistake).
* Restore long example descriptions
This restores the names of the test cases to the original.
