Issue #3230125: Improve profile feature cohesion #2478
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kingdutch
added
type: refactoring
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
3 times, most recently
from
5b0337b
to
4aa31b1
Compare
9 tasks
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
12 times, most recently
from
930a328
to
3643139
Compare
Kingdutch
added
needs work: commit cleanup
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
from
3643139
to
8c0c3bd
Compare
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
from
8c0c3bd
to
c58dd56
Compare
Kingdutch
removed
the
needs work: commit cleanup
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
from
9e26c2d
to
36a612b
Compare
To make our code simpler we require that a profile exists and use hook_user_insert to make sure this happens regardless of how the user is created. However there was an issue in the profile module that caused a profile to be saved after use creation and its related hooks were completed. Depending on how we responded to that it would cause the profile not to be created at all or it would cause a duplicate profile to be created which then still failed being default. This change includes a patch to optimize the way profiles are created in the profile module during user registration which also ensures the profile is available during welcome e-mail sending.
We want to protect the overview of users on platforms which is why we introduced the `list user` permission which is also coming from Drupal issue #2953566. Unverified users were already not allowed to view the user search but they were still allowed to search for users through the "all" search which is inconsistent. This commit ensures that unverified users also can't use the all search to enumerate users. We need a separate check for this with respect to anonymous users because unverified users are able to see user profiles with the fields that can be seen by unverified users whereas anonymous users can't view profiles at all.
This was originally built because customers wanted real names to be public while still providing the user with the ability to hide their real name behind a nickname. However implementing this same logic in the current system has become quite complicated. Users now also have the ability to control the visibility of their profile fields more specifically which allows them to also hide their real name behind a nickname if the site managers give them the ability to do so. We ensure an upgrade path is included to make sure the behaviour doesn't unexpectedly change. This change reveals that the autocomplete still needs work to ensure it works with the new permission system. We should probably change the autocomplete to use the user search instead.
We expand our wordlist to include newly used words and the configuration is updated to ignore Drupal's mangled field names in configuration for the social_profile module.
The group update adds the variation cache module however code in the group module is triggered during other updates which relies on that module, causing a fatal error. For background see https://www.drupal.org/project/group/issues/3134690 https://www.drupal.org/project/group/issues/3135757 https://www.drupal.org/project/drupal/issues/3150512 See 0adf6c0
This moves the widget and formatter functionality from social_tagging into a social_tag_split module. This allows it to also be reused by other functionality like the social_profile's profile tagging, without having to enable the social_tagging module which comes with content tagging.
This commit removes the profile tag settings and brings the feature in line with how content tagging works. We always enable the tag splitting because the no-split scenario can be emulated by only creating a single category. The access management can now be performed through the generic field settings and no longer requires special settings. The `use_category_parent` existed to allow searching for profiles with “any tag in category” which doesn’t require selecting a special value during editing but can be done during search using the facets widget.
In a9f41db we ensured that search queries required the `list user` permission so that search can not be used to enumerate users. This was needed because otherwise users might show up on the "All" search page. The user search still had a custom permission. However we don't actually expose a configuration page for that permission. So we change the view to use the permission that is actually needed in the underlying data system and we remove the special purpose permission.
We had initially deprecated the SocialProfileTagSplitWidget in 12.0.0 for removal in 13.0.0. However, it relies on the SocialProfileTagService for which we have already migrated and cleaned up the config, so in practice those systems already don't work. I think it's better to remove the code and have a hard-break than keep the code around in a non-functioning matter creating subtle bugs for people. I also don't want anyone to accidentally build on or extend the old system. Unfortunately it is not possible to deprecate these things in 11.x already because the replacement can't be introduced until 12.0.0. This makes the break a bit harder but we'll write an update guide to help out developers.
The required setting for the profile tag field wasn't propagated to the subfields that were being created.
This has been quite the challenge. There is code that's unaware of the existence of profiles, but that inadvertently triggers code that requires profiles. For example the `ginvite` module needs to send notifications when a user is created and does this in `hook_user_insert`. However this includes the display name of the account to address the user. We want to do this with their name if they've filled out this information on the registration page. We use module_implements_alter to change the order in which hooks fire to make sure the profile and social_profile hooks are always run first. This ensures whenever a user is saved, a profile will exist (together with the profile patch added earlier) and makes sure the user's name is properly generated if available. A backtrace is added to logging because figuring all this out without it was a pain.
We already had this styling in our FollowTag formatting, but it's actually a universal design pattern for tags in Open Social that they're displayed as small pills. To accomplish this we add the classes in our TagSplitFormatter so that themes can use this. This is accompanied by a field file change in socialbase that ensures the main label for the profile tag field is not shown but the individual labels of the subfields are rendered as labels instead.
When the role was introduced we installed new sites with "Verified user" capitalization but the update introduced it with "Verified User". In case the roles are used in labels that are clicked in tests, these differences in capitalization are problematic, and it also just doesn't look the way we intend for a large part of our customerbase. Our designer gave the following answer: "Based on our microcopy approach (Slite/Humans/Product language) we don’t use Title Case, which means that the right option is ‘Verified user’." This commit updates existing sites and pulls them in line with the correct spelling already used in new sites.
It's a bit weird to add a patch for something we manage. However we have a chicken/egg problem. We can't add the new twig file to socialbase because it'll break the existing profile tagging functionality. However, we require it to properly display the new profile tagging display structure and pass our tests. We include the patch until the profile refactor is released, then we can tag a new socialbase version with the fix and update within Open Social while removing the patch.
Profile tagging was something that was allowed for content managers. So in our upgrade path the content managers get the permission by default. This breaks our test which expects the content manager not to have access to any fields and thus see an access denied page.
The subfield settings upgrade path did not take the scenario where the social_profile_fields module was disabled. We implement the fallback to act as if the module was disabled and use the default address field behaviour.
Kingdutch
force-pushed
the
feature/improve-profile-feature-cohesion
branch
from
0069c50
to
dbacd5d
Compare
Kingdutch
added
status: needs review
Kingdutch
merged commit 5650d70
into
issue/3348399-3348407-3353443-solr-unified-search-with-content-tagging
189 checks passed
|
Removed milestone since this likely will be postponed to 14.x |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem/Motivation
Open Social has various modules that handle functionality related to user profiles (e.g.social_profile_privacyandsocial_profile_fields). Most of the functionality is optional but has a big impact on the way sites work when they're enabled. This requires modules that extend profile functionality to implement multiple paths depending on what configuration of modules is enabled.Additionally each module implements their own features in a way that does not properly utilise Drupal's access and fields system but instead relies on form and display alter hooks.
Finally because the functionality is split among several modules, configuration forms are either split out over multiple pages or require using form alter hooks, leading to a poor user and developer experience.
Proposed resolution
Merge the functionality of thesocial_profile_fields,social_profile_privacyand our internalsocial_registration_fieldsmodule into thesocial_profilemodule.Create a unified configuration screen that allows a site manager to edit whether a profile field is enabled, who can see it, who can edit it, whether it's required and whether it should be shown on the registration field.
Use Drupal's fields and permission system to handle the different options. For every editable profile field a "visibility" field is added to the profile as well. This is used to store the per-user visibility of their fields (this allows altering database or search queries based on field visibility) (this data was previously stored in users.data).
Issue tracker
https://www.drupal.org/project/social/issues/3230125
TODO
hide-profile-email.feature-- which was removed -- has changed in that a user viewing their own profile would not see their email if they hid it, but the new access levels makes this difficult, so a different indication is needed to show they can always see their own email)How to test
TODO
Screenshots
TODO
Release notes
TODO
Change Record
Requires multiple change-records, Todo:
view debug informationpermissionaccess user profileswhich is needed to view teaser profile information. However a new permission is required forlist usersto prevent access to pages like search or GraphQL lists and to disallow enumerating all users on a platform.social_profile_fieldssocial_profile_privacysocial_profile_registrationinsocial_profileuser_informationview has been removed, routeview.user_information.user_informationis replaced withsocial_profile.view_user_profile. Profile is now rendered usingfullview mode.\Drupal\social_user\Controller\SocialUserController::accessUsersPagesis removed and should be replaced with_entity_access: user.view. Access checking now properly happens on an entity level.Translations
Translatable strings are always extracted from the latest development branch. To ensure translations remain available for platforms running older versions of Open Social the original string should be added to
translations.phpwhen it's changed or removed.translations.phpfile.