Break apart workshop.jsx #31990

islemaster · 2019-11-19T01:58:15Z

Break the large and fairly entangled Workshop component in workshop.jsx out into many small components. I'm doing this cleanup work in preparation for a behavior change that we haven't scheduled yet but is certainly badly needed: Letting workshop admins change names on enrollments (Jira). We're up to at least five requests for engineers to do this manually, so it's time to make some progress on it.

This change should be a pure refactor.

One benefit of breaking the components apart this way is that they show up with useful names in the React devtools for Chrome:

Testing story

There was very little existing coverage of this code. I only found the eyes test workshop_dashboard.feature.

I've added enough unit tests to achieve at least 70% coverage for every extracted component, which should be a big improvement since this is all code that had zero coverage before. Here's unit test coverage for this directory before (Notice that workshop.jsx isn't even on the list):

And here's coverage after:

I've also been doing manual testing of key behaviors on the workshop view throughout the process.

Reviewer Checklist:

Tests provide adequate coverage
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

islemaster · 2019-11-20T20:12:12Z

apps/test/unit/code-studio/pd/workshop_dashboard/workshopFactory.js

+  .attr('first_name', 'George')
+  .attr('last_name', 'Dragon')
+  .attr('school', 'Academy')
+  .attr('attended', false);


@madelynkasula I ❤️ rosie so far. Thank you for adding this!

yayy -- thank you for recommending it!

islemaster · 2019-11-20T20:13:18Z

apps/src/code-studio/pd/workshop_dashboard/workshop.jsx

@@ -9,37 +9,15 @@ import _ from 'lodash';
 import PropTypes from 'prop-types';


GitHub wants to hide this diff because it's too large, but please take a look - simplifying this component is why I think this change is worthwhile.

islemaster · 2019-11-20T20:14:48Z

apps/src/code-studio/pd/workshop_dashboard/EnrollmentsPanel.jsx

+/**
+ * View and manage the list of teachers enrolled in a workshop.
+ */
+export default class EnrollmentsPanel extends React.Component {


Note: I'm especially excited about extracting EnrollmentsPanel because this is where we'll give admins the ability to change enrollment details in the near future. There's definitely some further simplification that could happen in this component but I picked this as a stopping point since we'll be coming back to it soon anyway.

islemaster · 2019-11-20T20:16:44Z

Note to reviewers: There's no timeline on this and it's a large review, so don't feel a need to look at this right away.

bencodeorg · 2019-12-07T03:25:26Z

This is potentially obvious to PLC people or to people with more React experience (and I think is much more clear in the refactored version), but in case helpful to others here's a table of how I think each of these components get used depending on the status of the workshop:

hacodeorg · 2019-12-18T02:59:33Z

apps/src/code-studio/pd/workshop_dashboard/DetailsPanel.jsx

+    const {view, workshop, isWorkshopAdmin, onWorkshopSaved} = this.props;
+
+    if (view === 'edit') {
+      const header = <EditHeader handleSave={this.handleSaveClick} />;


Not a blocker. These headers, EditHeader, NotStartedHeader, AdminHeader, DetailsPanelHeader, have multiple nested layers to display a simple text + a button. And what really changes is the button name and its callback function. In this case, I would prefer a flatter component structure (maybe more verbose code) but easier to reason.

hacodeorg · 2019-12-18T03:08:19Z

apps/src/code-studio/pd/workshop_dashboard/AttendancePanel.jsx

+/**
+ * Provides controls for taking attendance in a workshop session.
+ */
+export default class AttendancePanel extends React.Component {


I see that we change file name style, for example AttendancePanel.jsx (new) vs. workshop_enrollment.jsx (old). What is the convention to follow?

I'd like us to move to PascalCase instead of snake_case for JavaScript filenames, as this is the convention other cabals are using.

hacodeorg

I've just reviewed half of the changes, so far so good :)

hacodeorg · 2019-12-18T03:13:45Z

apps/src/code-studio/pd/workshop_dashboard/WorkshopPanel.jsx

+ * Simple wrapper component for a chunk of related content when
+ * viewing a workshop.
+ */
+export default class WorkshopPanel extends React.Component {


This is a great UI abstraction!

hacodeorg

Phew, done, this PR is a good read. I learn a lot more about workshop page now.

hacodeorg · 2019-12-18T22:21:07Z

apps/test/unit/code-studio/pd/workshop_dashboard/EndWorkshopPanelTest.jsx

+    server = null;
+  });
+
+  it('when ready to close', () => {


What are the purpose of this test and the one below it? Do they test if a component can mount or if a component can be rendered?

It's a quick check that the component renders without errors when given a certain set of props. (By default, our tests fail when console.warn or console.error is called. One common way this happens for React components is PropType failures.) While these definitely are not robust in terms of checking a behavior for the component, I wanted to make sure all possible render paths were covered.

hacodeorg · 2019-12-18T22:31:39Z

apps/test/unit/code-studio/pd/workshop_dashboard/EnrollmentsPanelTest.jsx

+        loadEnrollments={loadEnrollments}
+      />
+    );
+    assert(


Nit: To be symmetrical to the test above, you can assert that spinner is not rendered here.

Great point! I'll do that.

hacodeorg · 2019-12-18T22:45:04Z

apps/test/util/testUtils.js

@@ -385,3 +385,45 @@ export function enforceDocumentBodyCleanup(
    describe('', runTestCases);
  });
 }
+
+/**
+ * Call in the `describe` block for a group of tests to stub the value of window.dashboard safely.


🥇 for the comments!

hacodeorg

🎉

Restores behavior introduced in #32453 and regressed during a complicated rebase in #31990. Most of the change was preserved, but the JSX that actually renders the new component was accidentally dropped. I mentioned in the previous PR that I wasn't adding a test for the UI because I was adding so many tests in the other PR (that regressed this!) and it would be covered. Turns out I was wrong! So now I'm adding a unit test for the Workshop component that verifies that we show the created_at date as expected.

islemaster commented Nov 20, 2019

View reviewed changes

islemaster requested review from breville and hacodeorg November 20, 2019 20:15

islemaster added refactor Better code, no behavior change code-hygiene labels Nov 20, 2019

islemaster marked this pull request as ready for review November 22, 2019 16:51

islemaster changed the base branch from staging to staging-next November 25, 2019 18:22

islemaster requested a review from bencodeorg December 6, 2019 19:58

islemaster mentioned this pull request Dec 17, 2019

Show workshop created_at date on workshop dashboard #32453

Merged

7 tasks

islemaster changed the base branch from staging-next to staging December 18, 2019 01:14

hacodeorg reviewed Dec 18, 2019

View reviewed changes

islemaster added 13 commits December 18, 2019 18:11

Extract Panel as a functional component

26023a7

Turn renderSignupPanel into a component

2dc3bbe

Simplify SignUpPanel

a9706a0

Extract IntroPanel component

6085c07

Move responsibility for rendering SignUpPanel into Workshop

d9eb462

Move WorkshopPanel to its own file

9771902

Pull IntroPanel into its own file

6edbf73

Move SignUpPanel to its own file

e357dd3

Fix loadWorkshop bug in IntroPanel

755e127

Extract AttendancePanel component

22786aa

Move AttendancePanel to its own file

80f7532

Clean up Workshop.render a bit

365c08c

Move rendering responsiblity for end workshop panel into render method

06659d8

islemaster added 23 commits December 18, 2019 18:19

Extract DetailsPanel component

413278b

Move DetailsPanel to its own file

d06e89f

Add jQuery dependency

3954798

Brittle rendering tests for DetailsPanel

0f9a07e

Simplify render method

2f140a4

Autobind loadWorkshop and loadEnrollments

637835e

Class comment

3b42903

Clean up locals a bit

f202402

Replace brittle tests with behavior tests

5efaa05

Deduplicate some render code

0d5416f

Replace more brittle tests

dd7ef43

Extract another render helper

bebc21c

Stub AttendancePanelTest

40728e7

Stub EndWorkshopPanelTest

3522aa1

Stub EnrollmentsPanelTest

7601f34

Stub IntroPanelTest

6fea83d

Add SignUpPanelTest

9260a21

Better coverage from AttendancePanelTest

2690683

Better coverage from EndWorkshopPanelTest

835bf01

EnrollmentsPanelTest covers render method better

371ec0b

Better coverage from EnrollmentsPanelTest

7db462a

Better coverage from IntroPanelTest

da9c3ac

Parallel assertion about spinner not being rendered

9d647cb

islemaster force-pushed the change-your-own-darn-enrollment branch from 8919629 to 9d647cb Compare December 19, 2019 02:22

hacodeorg approved these changes Dec 19, 2019

View reviewed changes

Fix imports after rebase

4ca028c

islemaster merged commit ebc2633 into staging Dec 19, 2019

islemaster deleted the change-your-own-darn-enrollment branch December 19, 2019 21:06

islemaster mentioned this pull request Dec 20, 2019

Restore created_at on workshop dashboard #32506

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break apart workshop.jsx #31990

Break apart workshop.jsx #31990

islemaster commented Nov 19, 2019 •

edited

islemaster Nov 20, 2019

maddiedierker Nov 20, 2019

islemaster Nov 20, 2019

islemaster Nov 20, 2019

islemaster commented Nov 20, 2019

bencodeorg commented Dec 7, 2019 •

edited

hacodeorg Dec 18, 2019

hacodeorg Dec 18, 2019

islemaster Dec 19, 2019

hacodeorg left a comment

hacodeorg Dec 18, 2019

hacodeorg left a comment

hacodeorg Dec 18, 2019

islemaster Dec 19, 2019

hacodeorg Dec 18, 2019

islemaster Dec 19, 2019

hacodeorg Dec 18, 2019

hacodeorg left a comment

		@@ -9,37 +9,15 @@ import _ from 'lodash';
		import PropTypes from 'prop-types';

Break apart workshop.jsx #31990

Break apart workshop.jsx #31990

Conversation

islemaster commented Nov 19, 2019 • edited

Testing story

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster commented Nov 20, 2019

bencodeorg commented Dec 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacodeorg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacodeorg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacodeorg left a comment

Choose a reason for hiding this comment

islemaster commented Nov 19, 2019 •

edited

bencodeorg commented Dec 7, 2019 •

edited