[B5] Migrate first reports to bootstrap 5! #35704
Conversation
biyeun
added
awaiting QA
There was a problem hiding this comment.
It is great to have the first reports migrated!
I didn't see anything specific that looks problematic, but also didn't spend a lot of time teasing out the B5-related changes from what looks like code cleanup/refactoring. Not approving since my review doesn't feel complete.
corehq/apps/reports/templates/reports/filters/bootstrap5/multi_option.html
Outdated
Show resolved
Hide resolved
| res = [] | ||
| #the NUMBER of cols sorting | ||
| sort_cols = int(self.request.GET.get('iSortingCols', 0)) | ||
| if sort_cols > 0: | ||
| for x in range(sort_cols): | ||
| col_key = 'iSortCol_%d' % x | ||
| sort_dir = self.request.GET['sSortDir_%d' % x] | ||
| col_id = int(self.request.GET[col_key]) | ||
| col = self.headers.header[col_id] | ||
| sort_prop = getattr(col, sort_prop_name) or col.prop_name | ||
| if x == 0: | ||
| self.primary_sort_prop = sort_prop | ||
| if self.selected_app_id: | ||
| sort_dict = { | ||
| sort_prop: { | ||
| "order": sort_dir, | ||
| "nested_filter": { | ||
| "term": { | ||
| self.sort_filter: self.selected_app_id | ||
| } | ||
| } | ||
| } | ||
| block = [] | ||
| for col in self.datatables_params.order: | ||
| col_ind = col['column'] | ||
| sort_dir = col['dir'] | ||
| dt_column_obj = self.headers.header[col_ind] | ||
| sort_prop = getattr(dt_column_obj, sort_prop_name) or dt_column_obj.prop_name | ||
| if col_ind == 0: | ||
| # this feels like a bit of a hack, but kept it in from the original sorting block | ||
| # prior to bootstrap 5 migration. could use a second look in the future | ||
| self.primary_sort_prop = sort_prop | ||
| if self.selected_app_id: | ||
| sort_dict = self._get_selected_app_sort_dict(sort_prop, sort_dir) | ||
| else: | ||
| sort_dict = {sort_prop: sort_dir} | ||
| block.append(sort_dict) | ||
| if len(block) == 0 and self.default_sort is not None: | ||
| block.append(self.default_sort) | ||
| return block |
There was a problem hiding this comment.
Difficult to tell what changed here that is B5 related and what is code refactoring. Could those be split into separate commits or is this necessarily a complicated transformation for B5 support?
There was a problem hiding this comment.
the whole sorting block changed, I'm not sure I can split it up any further? There is no other way to have a diff
There was a problem hiding this comment.
also FYI this isn't B5 related but Datatables 1.10+ upgrade related, so there isn't going to ever be a clean-cut diff as your would expect with property changes for B5-type changes.
our old version of datatables is not compatible with bootstrap 5, which is why the upgrade was necessary. So any b5 migration of a report, will also require these kind of sorting block updates (if there is a custom sorting block) due to datatables. Relevant context is part of this PR, which we went over as a group #35648
There was a problem hiding this comment.
I guess i could have split out _get_selected_app_sort_dict, but that was the only refactoring related change, and I did that at the end to make the whole sorting block a little more readable. Unlikely I would have changed it from the beginning. Everything else was not a refactor
corehq/apps/reports/templates/reports/filters/bootstrap5/multi_option.html
Outdated
Show resolved
Hide resolved
corehq/apps/reports/templates/reports/filters/bootstrap5/multi_option.html
Outdated
Show resolved
Hide resolved
corehq/apps/reports/templates/reports/filters/bootstrap5/multi_option.html
Outdated
Show resolved
Hide resolved
corehq/apps/reports/templates/reports/filters/bootstrap5/single_option.html
Outdated
Show resolved
Hide resolved
corehq/apps/reports/templates/reports/filters/bootstrap5/single_option.html
Outdated
Show resolved
Hide resolved
…_option.html Co-authored-by: Jenny Schweers <[email protected]>
|
thanks for the review @orangejenny and for catching those should be g2g now :) |
Technical Summary
Finally! I am so excited that we are here at last. This PR includes the migration of two reports to bootstrap 5:
Both have completed QA, and the related issues have been marked "done". Just waiting on the final sign-off:
https://dimagi.atlassian.net/browse/QA-7409
Safety Assurance
Safety story
QA on staging has taken place for these reports
Automated test coverage
diffs, yes
QA Plan
https://dimagi.atlassian.net/browse/QA-7409
Rollback instructions
leaving unchecked due to bootstrap 5 diffs, which can sometimes make a simple revert annoyingly complex depending on how much time has elapsed
Labels & Review