Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade select2 on domain app pages that use requirejs #22971

Merged
merged 9 commits into from Jan 16, 2019
Merged

Upgrade select2 on domain app pages that use requirejs #22971

merged 9 commits into from Jan 16, 2019

Conversation

orangejenny
Copy link
Contributor

These need to be migrated as a group because they all use the same code bundle on staging/prod (domain/js/bundle.js).

@esoergel / @Rohit25negi

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 14, 2019
@orangejenny
Upgraded pro bono pages
854e3c5
@orangejenny
Upgraded product new/edit page.
ecd96cb 
BaseCommTrackManageView was using @use_select2, but the only subclasses that
need it are NewProductView and EWSUserExtensionView. NewProductView is an easy upgrade;
EWSUserExtensionView can be upgraded later.
@orangejenny
Upgraded basic project settings page
6a55d44
@orangejenny
Renamed accounting/js/widgets to accounting/js/widgets_v3
b603c74
@orangejenny
Removed select2 dependency from InternalManagementSubscriptionView
8d010c2 
None of the three INTERNAL_SUBSCRIPTION_MANAGEMENT_FORMS used on this page use select2.
@orangejenny
Upgraded billing info pages
93c8c7f
@orangejenny orangejenny added the Open for review: do not merge A work in progress label Jan 14, 2019
@orangejenny
Migrated accounting accounts pages.
e0435e5 
Necessary because the last commit modified BillingAccountBasicForm
and these views also use that form.
@orangejenny
Copy link
Contributor Author

Or @nickpell because a good deal of this is billing code.

@orangejenny
Upgraded ConfirmSubscriptionRenewalView
ae95cf6 
Necessary because this uses the ConfirmSubscriptionRenewalForm which extends
the EditBillingAccountInfoForm which was modified when upgrading the domain
billing views.
@orangejenny
Upgraded ConfirmBillingAccountForExtraUsersView
c31c40d 
Needed because this view uses the ConfirmExtraUserChargesForm which
descends from the EditBillingAccountInfoForm which was affected
when domain billing pages were upgraded.
@orangejenny orangejenny removed the Open for review: do not merge A work in progress label Jan 15, 2019
@orangejenny
Copy link
Contributor Author

Did some testing on staging and upgraded a few more views that were affected by earlier commits.

This is an intricate migration because it can require changes to the js select2 API (options were renamed), the HTML markup (you have to call .select2() on a real select element now, whereas previously we usually used hidden inputs), and the python saving code (because for multiselects, the selected value is now represented by an array of strings rather than a single comma-separated string). So when one page is upgraded, any pages that share either the same django form or the same js code also have to be upgraded.

In any case, I'm done testing this and think it's in good shape for review & merge.

nickpell
nickpell approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@nickpell nickpell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was a lot of work. Good to see us modernizing our select2 widgets.

@orangejenny orangejenny merged commit a121c7c into master Jan 16, 2019
@orangejenny orangejenny deleted the jls/domain-select2-v4 branch January 16, 2019 18:49
orangejenny added a commit that referenced this pull request Jan 17, 2019
@orangejenny
Removed @use_select2 from views that don't have any select2s.
0d2d651 
I added this decorator to most accounting views in
#22971
to be safe, without taking the time to pick out which views need it.
These ones don't.
orangejenny added a commit that referenced this pull request Jan 17, 2019
@orangejenny
Removed @use_select2 from views that don't have any select2s.
6226efe 
I added this decorator to most accounting views in
#22971
to be safe, without taking the time to pick out which views need it.
These ones don't.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickpell nickpell nickpell approved these changes

Assignees
No one assigned
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@orangejenny @nickpell