Page MenuHomePhabricator

Fix an issue with editing pre-space objects using a form with no visibility controls
ClosedPublic

Authored by epriestley on Feb 18 2016, 5:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 9:53 PM
Unknown Object (File)
Fri, Dec 6, 8:28 AM
Unknown Object (File)
Tue, Nov 26, 9:09 PM
Unknown Object (File)
Fri, Nov 22, 4:54 AM
Unknown Object (File)
Nov 18 2024, 9:39 AM
Unknown Object (File)
Nov 14 2024, 1:00 PM
Unknown Object (File)
Nov 10 2024, 1:20 PM
Unknown Object (File)
Nov 6 2024, 5:04 AM
Subscribers
Tokens
"Love" token, awarded by amusso.

Details

Summary

WMF ran into this after their update. Here's the setup:

  • When you enable Spaces, we leave all existing objects set to null, which means "these belong to the default space". This is so we don't have to go update a trillion objects.
  • New objects get set to the default space explicitly (PHID-SPCE-...) but older ones stay with null.
  • If you edit an older object (like a task) from the time before Spaces, and the form doesn't have a Visbility/Spaces control, we would incorrectly poplate the value with null when the effective value should be the default space PHID.
  • This caused a "You must choose a space." error in the UI.

Instead, populate the control with the effective value instead of the literal database value. This makes the edit go through cleanly.

Also add a note about this for future-me.

Test Plan
  • Disabled "Visibility" control in task edit form.
  • Edited an old task which had null as a spacePHID in the database.
  • Before patch: UI error about selecting a Space.
  • After patch: edit goes through cleanly.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix an issue with editing pre-space objects using a form with no visibility controls.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
104–105

This basically boils down to:

$space_phid = $object->getSpacePHID();
if ($space_phid) {
  return $space_phid;
}

// This is the meaning of `null`.
return "THE DEFAULT SPACE PHID";
chad edited edge metadata.
This revision is now accepted and ready to land.Feb 18 2016, 6:07 PM

WMF is verifying this on their end, I'll kick it in once they confirm it.

Works now at least on the task I had issues with.

Looks like it worked, I'll wait for one more confirmation to be sure.

got a second confirmation. it's fixed :)

Thanks everyone for such a quick fix!

This revision was automatically updated to reflect the committed changes.