1109721 - Fold rbmozui into mozreview

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

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.

Comment 1

[Steven MacLeod [:smacleod]]

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.

Comment 2

[Mark Côté [:mcote]]

We should move everything to the mozreview extension.

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: 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

Pull down these commits:

hg pull review -r 8553757848f1f5ee536a14704943eb19330d94b3

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5887/#review4847

Ship It!

Comment 5

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5889/#review4849

Ship It!

Comment 6

[Gregory Szorc [:gps]]

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

You missed a reference in `create-test-environment`.

Comment 7

[Mauro Doglio [:mdoglio]]

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 8

[Mauro Doglio [:mdoglio]]

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

ooops! fixed now

Comment 9

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5891/#review4877

Ship It!

Comment 10

[Mauro Doglio [:mdoglio]]

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 11

[Mauro Doglio [:mdoglio]]

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

Comment 12

[Gregory Szorc [:gps]]

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.

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5875/#review5037

Comment 14

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5877/#review5039

Looks like a bunch of code moves. Rubber stamp.

Comment 15

[Gregory Szorc [:gps]]

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.

Comment 16

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5881/#review5043

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5883/#review5045

Ship It!

Comment 18

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5887/#review5047

Ship It!

Comment 19

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5889/#review5049

Ship It!

Comment 20

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5891/#review5051

Ship It!

Comment 21

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5975/#review5053

Ship It!

Comment 22

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5977/#review5055

Ship It!

Comment 23

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5979/#review5057

Ship It!

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6071/#review5059

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

Why did you remove this?

Comment 25

[Gregory Szorc [:gps]]

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?

Comment 26

[Mauro Doglio [:mdoglio]]

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 27

[Gregory Szorc [:gps]]

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?

Comment 28

[Mauro Doglio [:mdoglio]]

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?

Comment 29

[Gregory Szorc [:gps]]

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.

Comment 30

[Gregory Szorc [:gps]]

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.

Comment 31

[Mauro Doglio [:mdoglio]]

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 32

[Mauro Doglio [:mdoglio]]

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

Comment 33

[Mauro Doglio [:mdoglio]]

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.

Comment 34

[Mauro Doglio [:mdoglio]]

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 35

[Mauro Doglio [:mdoglio]]

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 36

[Mauro Doglio [:mdoglio]]

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

Comment 37

[Mauro Doglio [:mdoglio]]

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

Done in the last 2 commits

Comment 38

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6139/#review5151

Ship It!

Comment 39

[Gregory Szorc [:gps]]

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.

Comment 40

[Gregory Szorc [:gps]]

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.

Comment 42

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove rbmozui folder

Comment 43

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove references to rbmozui in create-test-environment

Comment 44

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Rename rbmozui templatetags to mozreview

Comment 45

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Rename rbmozui templates folder to mozreview

Comment 46

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove remaining references to rbmozui

Comment 47

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove remaining occurences of rbmozui

Comment 48

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Move rbmozui to mozreview (Bug 1109721) r=smacleod

Comment 49

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: remove/rename rbmozui occurences from docs

Comment 50

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Move rbmozui settings under mozreview

Comment 51

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Merge rbmozui and mozreview extension classes

Comment 52

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Move the remaining content of rbmozui to mozreview

Comment 53

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Fix mozreview egg generation

Comment 54

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Fix package_data definition in setup.py

Comment 55

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: move try.js to the review bundle

Comment 56

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove references to rbmozui-commits urls

Comment 57

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Remove rbmozui activation

Comment 58

[Mark Côté [:mcote]]

The commits for this bug actually only move rbmozui over to mozreview; I've filed bug 1262548 for rbbz.