Feedback and Errata for Global Settings
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	epriestley
	Jun 5 2016, 8:20 PM

Description

This is a followup from T4103. Administrators can now configure global settings.

If you run into issues or have questions about the new stuff, post them here.

These features, originally discussed in T4103, have not been built yet and are not expected to work:

Customizing default search queries available in applications. This is not implemented as a setting and didn't seem to be much of a priority for many installs.
Multiple different profiles ("Engineering" vs "Marketing"). There isn't much of a clear need for this right now, and figuring out how to get users into those profiles isn't completely straightforward.
Anything Dashboards-related. This stuff will roll into the next Dashboards iteration (T10390 or similar) instead.

Revisions and Commits

rP Phabricator
	D16118	rP2e450212503e Fix several issues with email-related global preferences
	D16117	rPcb7560d30172 Remove "re prefix" and "vary subjects" config
	D16112	rPd68b2cc0e4c4 Fix construction of default settings for users with no settings at all
	D16066	rP814fa135b036 Centralize "this is the current user for the request" code
	D16051	rPb4a07d528fdf Allow users to unset "Editor", tailor short error messages properly on…
	D16050	rP4936be0868bd Fix exception when trying to save global setting mail tags
	D16049	rP6ac5f84c8bc9 Redirect older "/panel/" settings URIs to modern location

Related Objects
Search...

		Status	Assigned	Task
		Resolved	epriestley	T11098 Feedback and Errata for Global Settings
		Resolved	None	T11223 404 site after Terminate Conduit API key

Event Timeline

epriestley created this task.Jun 5 2016, 8:20 PM

Herald added subscribers: faulconbridge, eadler. · View Herald TranscriptJun 5 2016, 8:20 PM

epriestley added a project: User Preferences.Jun 5 2016, 8:22 PM

epriestley mentioned this in T4103: Implement "Role Profiles" to provide search, homepage and application defaults.Jun 5 2016, 8:26 PM

epriestley added projects: Restricted Project, Prioritized.

epriestley moved this task from Backlog to Paused on the Prioritized board.

cspeckmim added a subscriber: cspeckmim.Jun 5 2016, 8:29 PM

Customize Menu... link in the homepage 404s.

Ah, thanks. We actually have a lot more of those than I realized.

epriestley added a revision: D16049: Redirect older "/panel/" settings URIs to modern location.Jun 5 2016, 8:36 PM

I get an exception trying to save the global Email Preferences (Argument 1 passed to PhabricatorEmailPreferencesSettingsPanel::getAllTags() must be an instance of PhabricatorUser, null given)

epriestley added a revision: D16050: Fix exception when trying to save global setting mail tags.Jun 5 2016, 8:41 PM

epriestley added a commit: rP6ac5f84c8bc9: Redirect older "/panel/" settings URIs to modern location.Jun 5 2016, 8:41 PM

Display Preferences gets very angry if Editor Link is empty ("Editor link has an invalid or missing protocol" + all fields are marked "invalid").

All other sections appears to work...

epriestley added a revision: D16051: Allow users to unset "Editor", tailor short error messages properly on settings forms.Jun 5 2016, 8:54 PM

epriestley added a commit: rP4936be0868bd: Fix exception when trying to save global setting mail tags.Jun 5 2016, 8:57 PM

epriestley added a commit: rPb4a07d528fdf: Allow users to unset "Editor", tailor short error messages properly on….Jun 5 2016, 9:03 PM

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jun 6 2016, 4:08 PM

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

Calls to .edit endpoints fail with cache exception:

echo '{ 	"objectIdentifier": 11098,    
	"transactions": [{ "type": "subscribers.add", "value": ["PHID-USER-lgbuqrsim4oysksuxv5b"]}] 
}'  | arc call-conduit maniphest.edit

against this install gives:
"ERR-CONDUIT-CORE: Attempting to access attached data on PhabricatorUser (via getUserSetting()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.\n\nData is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly."

Full stack from my dev:

0121] [2016-06-06 19:08:37] EXCEPTION: (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getUserSetting()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.\n\nData is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:1460]
[Mon Jun 06 19:08:37.405875 2016] [:error] [pid 20664] [client 10.249.29.100:40121] arcanist(head=master, ref.master=ca3324094259, custom=1), extension(head=master, ref.master=708b68e2a3c3), phabricator(head=master, ref.master=d1999557dca2), phutil(head=master, ref.master=557309b9242a), playground(head=master)
 #0 <#2> PhabricatorUser::requireCacheData(string) called at [<phabricator>/src/applications/people/storage/PhabricatorUser.php:484]
 #1 <#2> PhabricatorUser::getUserSetting(string) called at [<phabricator>/src/applications/policy/query/PhabricatorPolicyQuery.php:204]
 #2 <#2> PhabricatorPolicyQuery::loadObjectPolicyPHIDs() called at [<phabricator>/src/applications/policy/query/PhabricatorPolicyQuery.php:66]
 #3 <#2> PhabricatorPolicyQuery::loadPage() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:227]
 #4 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php:39]
 #5 <#2> PhabricatorPolicyEditEngineExtension::buildCustomEditFields(-----, -----) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:149]
 #6 <#2> PhabricatorEditEngine::buildEditFields(-------------) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1790]
 #7 <#2> PhabricatorEditEngine::buildConduitResponse(ConduitAPIRequest) called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngineAPIMethod.php:40]
 #8 <#2> PhabricatorEditEngineAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:122]
 #9 <#2> ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
 #10 <#2> ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
 #11 <#2> ConduitCall::execute() called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:81]
 #12 phlog(PhabricatorDataNotAttachedException) called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:101]
 #13 PhabricatorConduitAPIController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
 #14 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]

epriestley mentioned this in D16065: Fix one more remarkup line wrapping issue.Jun 7 2016, 1:54 PM

epriestley added a revision: D16066: Centralize "this is the current user for the request" code.Jun 7 2016, 2:00 PM

epriestley mentioned this in rP8a7ded61293a: Fix one more remarkup line wrapping issue.Jun 7 2016, 2:04 PM

epriestley added a commit: rP814fa135b036: Centralize "this is the current user for the request" code.Jun 7 2016, 2:43 PM

For completeness, T11107 was also related to this.

epriestley mentioned this in Blog Post: Development Notes (2016 Week 24).Jun 11 2016, 11:43 AM

We are seeing an issue where the global default email preferences are not reflected in the user email preferences.

Reproduction steps:

Phabricator version: https://secure.phabricator.com/rP63d413e20136df381899768a91aac06d6e12d4f3
libphutil version: https://secure.phabricator.com/rPHUfb1e159d36402cc5f6cdb64726599acf784283b6

Delete row related to my user from phabricator_user.user_preferences manually via SQL (if there is a better way let me know!)
Go to /settings/builtin/global/page/emailpreferences/ and set "A commit is created" to "Ignore"
bin/purge --purge-all
View my user email preferences (/settings/user/{username}/page/emailpreferences/)

Expected: My user's email preference for "A commit is created" is "Ignore"
Actual: My user's email preference for "A commit is created" is "Email"

It is entirely possible step 1 is flawed, apologies for not being able to test a new user yet (or one who does not already have email preferences saved in the database). Also, apologies as we have not yet been able to test whether the global settings is respected when it comes to actually deciding to send an email or not.

Let me know if I can provide anymore information, thanks!

Ah, I think I've got a repro. Does this "fix" it?

Go to any other preferences panel.
Click save.
Go back to "Email Preferences".

Yup, that did the trick!

Cool, pretty sure I've got a repro then -- real fix coming up in a second.

epriestley added a revision: D16112: Fix construction of default settings for users with no settings at all.Jun 13 2016, 10:23 PM

epriestley added a commit: rPd68b2cc0e4c4: Fix construction of default settings for users with no settings at all.Jun 13 2016, 10:50 PM

Confirmed it fixed the issue we were seeing, thanks for the quick response!

chad merged a task: T11149: user settings and global defaults for email do not reflect site config.Jun 14 2016, 5:19 PM

chad added subscribers: chad, allixsenos.

as per T11149, my email format setting defaults do not reflect active instance config

upgraded libphutil & phabricator to HEAD, still showing Vary Subjects as being Default: Vary when the config (and behavior) is obviously Do Not Vary.

the fix trick from @epriestley, saving some other pref panel (Email Delivery, specifically) did not fix the issue

Bug: The global email preferences are not being respected when Phabricator decides whether or not to send an email.

Steps to reproduce

Phabricator version: https://secure.phabricator.com/rP33ec855449dde85b3053bb22e59b9c55d6b7f810
libphutil version: https://secure.phabricator.com/rPHUfb1e159d36402cc5f6cdb64726599acf784283b6

Set /config/edit/metamta.mail-adapter/ to PhabricatorMailImplementationTestAdapter (we test in a staging environment and do not want to actually send the email)
Delete row related to my user from phabricator_user.user_preferences manually via SQL (if there is a better way let me know!)
Go to /settings/builtin/global/page/emailpreferences/ and set "A commit is created" to "Ignore"
bin/purge --purge-all
Make a commit to a repository tracked by Phabricator
Use bin/mail list-outbound to find emails related to the commit

Expected: Emails to my user to be marked as VOIDED.
Actual: Emails to my user are marked as SENT.

I also tried two other steps:

Saving a non-email related preference (timezone) for my user. Result: my user still had emails marked as SENT.
Saving my user's email preferences. Result: my user correctly had emails marked as VOIDED.

Let me know if I can provide any other information. Thanks!

epriestley added a revision: D16117: Remove "re prefix" and "vary subjects" config.Jun 14 2016, 6:30 PM

epriestley added a revision: D16118: Fix several issues with email-related global preferences.Jun 14 2016, 7:18 PM

@bizrad6 Thanks, I think D16118 should fix that once it lands. Sorry about these issues, the actual global stuff happened at the very end of the settings changes and I didn't test it as thoroughly as I should have.