Closed Bug 1109721 Opened 5 years ago Closed 5 years ago

Fold rbmozui into mozreview

Categories

(MozReview Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: mdoglio)

References

Details

Attachments

(16 files, 1 obsolete file)

39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
There are some common functions between rbbz and rbmozui, specifically those used to iterate child review requests of a squashed review request, that we could abstract out to some common library.
Common library is one option. Another option would be to have one of the extensions depend on the other. Review Board has built in support for extension dependencies that enforce installing, and enable extensions simultaneously.

If we went the dependency route I think it'd make sense for RBMozUI to depend on RBBZ.
We should move everything to the mozreview extension.
Summary: Reduce code duplication between rbbz and rbmozui → Fold rbbz and rbmozui into mozreview
Assignee: nobody → mdoglio
/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder

Pull down these commits:

hg pull review -r 8553757848f1f5ee536a14704943eb19330d94b3
Attachment #8581849 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/5871/#review4851

You missed a reference in `create-test-environment`.
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment

Pull down these commits:

hg pull review -r 27b80df5d5f6b1523b596489ba692de0ff811fa8
Attachment #8581849 - Flags: review?(gps)
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment

Pull down these commits:

hg pull review -r 27b80df5d5f6b1523b596489ba692de0ff811fa8
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment
/r/5975 - Remove remaining references to rbmozui
/r/5977 - Fix package_data definition in setup.py
/r/5979 - move try.js to the review bundle

Pull down these commits:

hg pull review -r e0b2d359f2a4dfd2983414da5928aeca0150119d
https://reviewboard.mozilla.org/r/5873/#review5035

This diff looks hairy. But I looked at the raw Mercurial commit and it is just a bunch of file moves with no actual code changes. So this gets a rubber stamp.
https://reviewboard.mozilla.org/r/5877/#review5039

Looks like a bunch of code moves. Rubber stamp.
https://reviewboard.mozilla.org/r/5879/#review5041

The raw Mercurial commit reveals this is a bunch of file moves plus deletion of pylib/mozreview/mozreview/rbmozui/__init__.py plus some minor import refactoring. LGTM.
https://reviewboard.mozilla.org/r/6071/#review5059

::: pylib/mozreview/setup.py
(Diff revision 1)
> -    package_data={

Why did you remove this?
https://reviewboard.mozilla.org/r/5885/#review5063

::: pylib/mozreview/mozreview/extension.py
(Diff revision 1)
> -        TemplateHook(self, 'base-css', 'mozreview/commits-stylings-css.html',
> -                     apply_to='rbmozui-commits')
>          TemplateHook(self, 'base-css', 'mozreview/review-stylings-css.html',
> -                     apply_to=review_request_url_names + ['rbmozui-commits'])
> +                     apply_to=review_request_url_names)

I'm not sure why this was done. The final state of this patch series still has references to "rbmozui-commits" in it. And, the commits-styling-css.html file still exists.

I don't see how this isn't regressing something due to losing CSS info. Am I missing something?
https://reviewboard.mozilla.org/r/6071/#review5065

> Why did you remove this?

I don't think we need it. I tried to generate the eggs without that and the files were in the package. So it's one thing less we need to keep up to date
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

https://reviewboard.mozilla.org/r/5871/#review5067

This commits series looks mostly good. I assume you will be collapsing all the commits before landing, since each commit isn't standalone?

Also, at the final state of this series, there are still numerous references to "rbmozui" throughout the source tree. Was it your intention to rename or remove these as well?
Attachment #8581849 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/5871/#review5069

Yeah, I'll do that.

For the remaining occurences of rbmozui, I wasn't planning to remove them. Should I?
https://reviewboard.mozilla.org/r/6071/#review5081

> I don't think we need it. I tried to generate the eggs without that and the files were in the package. So it's one thing less we need to keep up to date

We need it to perform packaging properly. Currently, things work by coincidence since we run `python setup.py develop` inside each extension. If we remove that (which we should - it's a hack), Review Board site generation fails spectacularly.

Please leave package_data in setup.py.
https://reviewboard.mozilla.org/r/5871/#review5083

It's a bit weird to have "rbmozui" references. But it isn't the end of the world. It would be a good follow-up work item.
https://reviewboard.mozilla.org/r/6071/#review5087

> We need it to perform packaging properly. Currently, things work by coincidence since we run `python setup.py develop` inside each extension. If we remove that (which we should - it's a hack), Review Board site generation fails spectacularly.
> 
> Please leave package_data in setup.py.

Oh now I see why it was working. I'll add it back
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment
/r/5975 - Remove remaining references to rbmozui
/r/5977 - Fix package_data definition in setup.py
/r/5979 - move try.js to the review bundle
/r/6071 - Fix mozreview egg generation

Pull down these commits:

hg pull review -r aa9a714a0485afe7d0cbd20a27aeac1bc81f66c3
Attachment #8581849 - Flags: review?(gps)
Attachment #8581849 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/5885/#review5109

> I'm not sure why this was done. The final state of this patch series still has references to "rbmozui-commits" in it. And, the commits-styling-css.html file still exists.
> 
> I don't see how this isn't regressing something due to losing CSS info. Am I missing something?

mconley told me it was a leftover fragment from the old Commits tab and I could get rid of it. I forgot to remove review-stylings-css.html and the commits bundle, I'll do it now.
Attachment #8581849 - Flags: review?(smacleod)
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment
/r/5975 - Remove remaining references to rbmozui
/r/5977 - Fix package_data definition in setup.py
/r/5979 - move try.js to the review bundle
/r/6071 - Fix mozreview egg generation
/r/6139 - Remove remaining occurences of rbmozui

Pull down these commits:

hg pull review -r 31ba99d666d5953e84f2724f2e159570187b4411
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment
/r/5975 - Remove remaining references to rbmozui
/r/5977 - Fix package_data definition in setup.py
/r/5979 - move try.js to the review bundle
/r/6071 - Fix mozreview egg generation
/r/6139 - Remove remaining occurences of rbmozui

Pull down these commits:

hg pull review -r ebdd2a4d7ccf83f7dad226a241313073f6275663
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

/r/5873 - Move rbmozui to mozreview (Bug 1109721) r=smacleod
/r/5875 - Move rbmozui settings under mozreview
/r/5877 - Merge rbmozui and mozreview extension classes
/r/5879 - Move the remaining content of rbmozui to mozreview
/r/5881 - Rename rbmozui templates folder to mozreview
/r/5883 - Rename rbmozui templatetags to mozreview
/r/5885 - Remove references to rbmozui-commits urls
/r/5887 - Remove rbmozui activation
/r/5889 - Remove rbmozui folder
/r/5891 - Remove references to rbmozui in create-test-environment
/r/5975 - Remove remaining references to rbmozui
/r/5977 - Fix package_data definition in setup.py
/r/5979 - move try.js to the review bundle
/r/6071 - Fix mozreview egg generation
/r/6139 - Remove remaining occurences of rbmozui
/r/6141 - remove/rename rbmozui occurences from docs

Pull down these commits:

hg pull review -r 1b538d250288107b28cba4ca345758310666b435
Blocks: 1144620
https://reviewboard.mozilla.org/r/6141/#review5153

::: docs/hacking-mozreview.rst
(Diff revision 1)
> -If you modify ``mozreview``, ``rbbz`` or ``rbmozui``, you'll need to produce
> +If you modify ``mozreview``, ``rbbz`` or ``mozreview``, you'll need to produce

mozreview is in here twice.
Attachment #8581849 - Flags: review?(gps) → review+
Comment on attachment 8581849 [details]
MozReview Request: bz://1109721/mdoglio

https://reviewboard.mozilla.org/r/5871/#review5155

This whole series looks good enough to land. We should test this pretty heavily on dev once it does.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8581849 - Flags: review?(smacleod)
Depends on: 1158908
No longer depends on: 1158908
Attachment #8581849 - Attachment is obsolete: true
Attachment #8618879 - Flags: review+
Attachment #8618880 - Flags: review+
Attachment #8618881 - Flags: review+
Attachment #8618882 - Flags: review+
Attachment #8618883 - Flags: review+
Attachment #8618884 - Flags: review+
Attachment #8618885 - Flags: review+
Attachment #8618886 - Flags: review+
Attachment #8618887 - Flags: review+
Attachment #8618888 - Flags: review+
Attachment #8618889 - Flags: review+
Attachment #8618890 - Flags: review+
Attachment #8618891 - Flags: review+
Attachment #8618892 - Flags: review+
Attachment #8618893 - Flags: review+
Attachment #8618894 - Flags: review+
Product: Developer Services → MozReview
The commits for this bug actually only move rbmozui over to mozreview; I've filed bug 1262548 for rbbz.
Summary: Fold rbbz and rbmozui into mozreview → Fold rbmozui into mozreview
You need to log in before you can comment on or make changes to this bug.