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)
Thu, Dec 19, 6:48 AM
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
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
Branch
space1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 10759
Build 13246: Run Core Tests
Build 13245: arc lint + arc unit

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.