Page MenuHomePhabricator

Diffusion + Herald - warn users if importing repository
ClosedPublic

Authored by btrahan on Apr 29 2014, 9:12 PM.
Tags
None
Referenced Files
F14035426: D8903.diff
Sun, Nov 10, 5:52 AM
F14034201: D8903.id21129.diff
Sat, Nov 9, 10:46 PM
F14011858: D8903.diff
Fri, Nov 1, 5:31 AM
F13980227: D8903.id21135.diff
Sat, Oct 19, 9:09 AM
F13980196: D8903.id21129.diff
Sat, Oct 19, 9:01 AM
F13959490: D8903.id.diff
Mon, Oct 14, 7:37 PM
Unknown Object (File)
Oct 1 2024, 12:42 AM
Unknown Object (File)
Sep 25 2024, 7:29 AM
Subscribers

Details

Summary

'cuz things fail a bunch until importing is done. Fixes T4094.

Test Plan

set isImporting to return true. Browsed Diffusion and saw helpful warnings everywhere. Browse Herald transcript and saw a helpful warning

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan retitled this revision from to Diffusion + Herald - warn users if importing repository.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/diffusion/controller/DiffusionController.php
48–57

Maybe throw (or at least phlog()) if we fail to insert? I don't see a cleaner way to do this right now, but it would be nice to get told if this breaks.

(Maybe we eventually separate $crumbs out so that we can just prepend the message.)

This revision is now accepted and ready to land.Apr 29 2014, 9:17 PM
src/applications/diffusion/controller/DiffusionController.php
48–57

I was trying to be fairly clever here as not all pages will have crumbs, and the ones that don't tend not to need such a message. An example is the commit edit controller.

I think maybe the best way is to explicitly build this warning on a per controller basis rather than automagically do it and show iff there is some crumbs UI. That said, I was torn on which would be easiest for the next developer, and the explicitly build version sounds like a pain.

Ah, that makes sense. Yeah, I don't see a clean way to do this right now and it doesn't seem important enough to go on a rampage restructuring everything.

btrahan updated this revision to Diff 21135.

Closed by commit rP7ed28dacb585 (authored by @btrahan).