* Adding logic to hide Create Post on empty dashboard
As implemented, folks won't see this page if they can't create a post.
However, as our implementation drifts, we should continue to apply the
same policy.
See forem/forem#16999 for the conditional logic that redirects users.
* Adding a hack to fix a unit test
* Encapsulating the "why we hide/show" something
Prior to this commit, we were relying on our views/templates to
understand why it might want to append the "hidden" dom class. That is
not ideal, because it required repeating knowledge about the conditions
of the policy outside of the policy's domain.
With this commit, we're removing a duplication of knowledge; or more
appropriately asking the steward responsible for the knowledge to
produce the answer.
* Refining the note to not describe a course of action
* Adjusts Member Index View tooltips to include resource type
* Adds .compact to the list of resource_types in #format_role_tooltip
* Updates users_helper.rb#format_role_tooltip tests
* right align tooltip
* Update app/views/admin/users/_member_index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* add an xl breakpoint and use in member index view
* expand search controls at medium breakpoint
* tweak small screen layout options per breakpoint, add overflow button
* small fix
* switch where role appears depending on breakpoint
* adjust for smallest layout
* tweaks for medium layout
* tweaks for large layout
* tweaks for XL table
* Revert "conditionally render create post button for admins #16490 (#16606)"
This reverts commit 1cb45995cb.
* Adding conditional create post rendering
This pull request does two things:
- Reverts the forem/forem#16606
- Replaces the conditional rendering with the approach from forem/forem#17076
What to consider:
- How does this impact Cumulative Layout Shift (CLS). Prior to this
commit, we didn't "flicker" the "Create Post" button into view if the
site didn't have the conditional.
- In reverting forem/forem#16606 we remove a dependency on Turbo.
- We reduce one network call, relying instead on the async_info to carry
the visible/hidden logic.
If the "flicker" is a problem we could add a conditional in the page for
if the site has enabled that feature. However, that's a place holder as
we're looking at possible other reasons for allowing/disabling the
create a post.
This is a non-blocking PR, as in we can merge or not merge this and
proceed with what we have. Instead the main goal is to "unify" our approach.
* Update cypress/integration/seededFlows/policyFlows/limitPostCreationToAdmins.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Leveraging change from forem/forem#17143
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Adding link to Spaces admin guide
This commit presently links to a missing page, but in consultation with
Ella, she'll be putting the page at the linked URL.
The original copy was:
> Want to learn more about spaces? Check [Admin Guide]
I chose to go with the following, as assistive technology will announce
the link's text as "more about Spaces in the Admin Guide", which I find
more a11y oriented.
> Learn [more about Spaces in the Admin Guide]
Closesforem/forem#17148
* Update app/views/admin/spaces/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Bringing the Learning inside the Linking
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Prior to this commit, we always assumed a policy based attribute would
have the "hidden" class and then we'd rely on JavaScript to toggle the
visibility.
With this approach, we are "cheating" a bit by specifically referencing
a feature flag that is the driver for the visibility behavior.
This mirrors the one-off behavior of forem/forem#16606. Namely, when
the flag is off, we render without default hiding. And when it's on, we
include the "hidden" attribute.
In both cases, we will still leveraged the [applyApplicationPolicyToggles.js][1]
introduced in forem/forem#17076.
An impact could be that if multiple flags are fiddling with the policy,
we could see an area disappear or wrongly remain present. This would be
a warning to fix (the server would still enforce correct policies so
this would be a bit of a "heads up we have a mismatch in understanding
that happens to impact UI behavior").
[1]:https://github.com/forem/forem/blob/main/app/javascript/packs/applyApplicationPolicyToggles.js
* Adds data to the member column in Member Index View
* Adds roles, statuses, and latest activity to Member Index views
* Adds the list of orgs a user belongs to in Member Index view
* Updates the image URL to link to each users member detail view
* Adds comment_suspended to user current_status and makes roles prettier
* Fixes bullet warning and encapsulates org logic in method
* Properly displays today and yesterday in words
* Refactors #current_organizations to use ternary
* Updates user.rb Member Index View-related methods
* Adjusts the styling of user roles and adds tooltip to roles
* Updates Member Index View to be mobile-friendly
* Updates userIndexView.spec.js
* Updates capitalization within userIndexView.spec.js
* Updates Member Index View per mobile designs and feedback
* Adds member status logic inline with SVGs
* Updates the status column for mobile layout
* Adjust current_role tooltips for Member Index View
* Adds organization name tooltip to member orgas
* Adds organization pics to Member Index View and cleans up helpers
* Addresses PR review feedback (Thanks, Suzanne!)
* Uses CSS in place of SVGs for member statuses
* Adds E2E test for and removes link to org page from Member Index View
* Addresses PR feedback comments by refactoring Member Index View code
* Removes leftover code in user.rb
* Update method that tooltips rely on
* Spells regular correctly :
* Adds a spec that tests Admin::UsersHelper#format_last_activity_timestamp
* Adjusts #cascading_high_level_roles logic and roles
* Adds a spec for #cascading_high_level_roles
* Adds a #last_activity spec to the User model
* Adjusts code a bit per PR feedback. Thanks, Ridhwana! :)
* Updates specs for Member Index View helpers
* Adds Timecop.freeze to help with timestamp failure in user_spec.rb
This commit adds instruction as to what's happening when a person
toggles on this feature. So as to not repeat knowlege, I added a
`NUMBER_OF_MINUTES_FOR_CACHE_EXPIRY` constant so the UI can reference that
instead of dropping a hard-coded 15 in the UI.
Closesforem/forem#17097
When a user does not have any posts NOR can they create posts, we are to
hide the "Search > My Posts" section.
I have chosen not to write a test for this as it's exercising a well
tested policy and we don't have a unit test for this view. As part of
forem/forem#17076 I introduced some cypress tests. But that feels like
quite a bit of overkill for this feature.
Closesforem/forem#16837
Related to forem/forem#16821
* 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, only those who could create posts could edit their confi
However, we are allowing users who can't create posts to edit posts. So
we want to allow for them to change their editor configuration.
Revisits
- forem/forem#16516
Supplants
- forem/forem#16778
Related to
- forem/forem#17123
- forem/forem#16908
* 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>
* Ensuring confirmation and flash message for spaces
This change delivers an accessible form, and while it doesn't remove the
Save button nor produce a modal it does deliver the minimum viable
functionality.
In conversations with Suzanne, we spent an hour looking at how we might
make an accessible modal with the checkbox and state management
required as we as removing save buttons.
There are two paths:
1. Extend the current, yet deprecated, stimulus modal controller.
2. Extend the Preact modal work done for the Members Detail View.
We spent about 15 minutes pursuing the stimulus modal controller route,
and realized how much cruft it added to the system. Not ideal and very
opaque in it's interaction.
The second one we talked about, but for our alloted time was inadequate
to begin further work. To deliver on that generalized preact modal will
require at least an afternoon of work.
Closesforem/forem#17032
* Update config/locales/controllers/admin/fr.yml
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
* Update app/views/admin/spaces/index.html.erb
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
* 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.
* 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>
* initial work to show the expand/collapse search and filter on member index
* replace icon with actual button
* simplify logic in pack
* add cypress tests
* make sure both fields part of same form
* add an indicator in mobile view when field has a value
* make sure empty params treated same as missing params
* make sure indicators stay in step with current user input
* partials for inputs
* rename pack file
* Add toggle button to 'New Forem Secret' form
* Display messages using i18n
* Fix from snake case to kebab case
* Fix a bug when the specified class did not exist
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
* pagination theme for new admin view
* align pagination widget correctly in desktop view
* remove bottom pagination
* re-add pagination to the bottom
* use block styling for the links
* Adjusting article copy to be more general
Our language regarding articles needs minor revisions to speak a bit
more generally about content. This follows on the features of
AuthN/AuthZ work to allow forem admins to configure their forems such
that a subset of their forem members may not have the ability to create
articles.
Closesforem/forem#16890Closesforem/forem#16891
* Update app/views/users/_notifications.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
