Best Practices for Coding and Architecture

This page provides in-depth discussion of how to code in various frameworks and libraries, and how to deploy that code, based on OpenCraft experience with past and current clients.

It augments the standards laid out in the OpenCraft handbook.

Reinventing the Wheel

If a free or inexpensive project exists with a compatible license which is regularly updated and it provides the functionality you need, use it!

Push back hard on requests for custom development of these existing tools, except for "plugin" style development which is expressly supported by the tool developers.

Before agreeing to do custom development, get solid, written answers to these questions:

1. What will you do if the tool changes in the future? Who is going to pay for fixing your custom code?
2. Who is responsible for the security of the custom code?
3. What happens to the budget and/or whole project if a custom-built integration is more complex than the initial discovery estimated?

Upstreaming is always an option, but make sure the upstream developers welcome community contributions before offering it to clients.

React.js

React is a JavaScript library/framework. The basic idea of it is to write "components" which are JavaScript objects responsible for rendering themselves to the DOM based on a) the html they contain, b) the child components they contain, c) the "state" variables they have, d) the "prop" variables they were passed from their parent component.

When coding React components, please keep the following in mind:

• All components should subclass React.PureComponent or be written as functions (React.FunctionComponent).
  • React.PureComponent makes assumptions about state that simplify writing code
• All component props and redux state variables that are complex objects should be immutable (enforced via TypeScript, if you are using TypeScript, by declaring them as ReadOnlyArray<T>, ReadOnlySet<T>, and ReadOnly<T>, mutated using immutability-helper or plain ES6).
  • This has to do with rendering: in React.PureComponent, React does shallow comparison to see if a component should update. Therefore, if a complex object changes, React will not notice.
• Write sensible tests, including unit tests, snapshot tests, and/or end-to-end tests.
  • When reviewing changes to snapshot tests, carefully review the HTML diff to ensure the changes are expected.
  • Test files should be located alongside the component they test (so Card.tsx is tested in Card.spec.tsx)
  • Never import jest/test related code in .ts files that are part of the application (only in .spec.tsx files); this avoids adding several megabytes of test code to the app bundle.
  • When in doubt, end-to-end tests and Enzyme behavior tests are preferred. Snapshot tests are still useful, but not as important as an end to end test or even a regular React component test that simulates user interaction with the component and then make assertions about the result.
• Prefer to split big components up into smaller components that get composed together.
• Use the Container Pattern
  • Don't write a FoobarComponent that loads Foobar data from the REST API then renders it; instead write a FoobarComponent that accepts Foobar data as a prop (so its props are never undefined), and then write a FoobarContainerComponent which loads the Foobar data from the REST API and then once it's loaded renders a <FoobarComponent data={foobarData}/>. This lets us test the presentation/UX separately from the API/backend, provides better separation of concerns, and reduces the need to write code that checks if the prop has data or not when rendering.
  • This abstraction layer might not be needed in the future with newer React features. Read about Suspense before designing a new project.
• Make sure the component is internationalized and accessible, as dicussed below.

React Internationalization

See the studio-frontend conventions to implement internationalization.

• Store messages for the component in a separate file called displayMessages.ts. For reference, see Header/displayMessages.ts
• Use defineMessage api to define messages file. An example of the file can be

import { defineMessages } from 'react-intl';

const messages = defineMessages({
    uiHeaderNavBarBrand: {
        id: 'uiHeaderNavBarBrand',
        defaultMessage: 'LabXchange',
        description: 'Title for Navbar.',
    }
});

• For each message component, define the following attributes:

  • id: It is the unique id of the message in camelCase convention. It is used to access the message in the component
  • defaultMessage: The message which will be displayed when no locale is specified or detected.
  • description: It is the helper text about the message, which tells what is the particular message for.

• Use FormattedMessage component from react-intl to use these messages.

• Write a wrapper such as createComponentWithIntl in order to test internationalized components.

Error & Success Alerts

Do not rely only on console.log to report when an error occurs; if (for example) your component fails to load data from an API or submit a form, it's important to display a user-visible error message such as a popup.

Create a logging component such as GlobalMessageReporter from the LX project to display (and log) user-friendly errors or basic success messages ("Changes saved!" etc.).

showErrorMessage(<WrappedMessage message={messages.itFailedMessage}/>, {
    title: <WrappedMessage message={messages.thereWasAProblem}/>,
    exception: err,
    details: `Exception when trying to process user ${user.id}`, // Shown only in console
});

Accessibility

At minimum, all OpenCraft websites must conform to the accessibility standards laid out by WCAG 2.1 levels A and AA, and where practically possible, level AAA.

Please read the following two documents:

• edX Accessibility Guidelines provides some general guidelines for writing accessible code.
• Reactjs: Accessibility provides a good summary of a11y issues common to React applications, as well as a number of useful development tools for assessing maintaining a11y, including: inspectors for the a11y tree, and several screen readers which can be used by different OS/browser combinations.

Rob Dodson has some nice a11y videos on youtube. These two are especially recommended:

• Screen Reader Basics: VoiceOver
• How I do an accessibility check

For a more comprehensive overview of a11y related topics for developers, the Web Accessibility course on Udacity is pretty good.

• WAI-ARIA Authoring Practices 1.1: useful advice ("No ARIA is better than Bad ARIA!") and accessibility development principles, including guides on how to use aria-role appropriately.
• WCAG checklist: useful lists of what to look for when creating accessible websites.
• WCAG 2.1 AA Requirements: toggle "Level AAA" checkbox in the left sidebar to display AAA level requirements as well
• Paragon: edX repo providing accessible React UI components

As with all OpenCraft PR/MR reviews, every code review must include an accessibility check. To test accessibility (a11y) effectively, manual steps are required, but can be assisted by some automated tools.

• eslint-plugin-jsx=a11y
  • Use this.performAccessibilityAudit() inside a View to manually trigger an accessibility audit. You should do that after performing actions which change the state of the page, for example after clicking a button that triggers a modal dialog.
• axe-core
• The axe plugin (chrome / firefox) helps find WCAG 2 and Section 508 accessibility defects on web applications.

Backend Architecture

There are two different architecture styles for the backend to a JavaScript web frontend.

In one version, the JavaScript calls APIs on a single backend. If data is needed from other APIs, that backend will fetch it and return the results.

In another version, the JavaScript calls each API from every service and assembles the results in the browser.

Neither is inherently superior, but consistency and planning will save you a lot of headache down the road.

Some components to keep in mind:

• An identity provider. Who handles login/registration and basic user account data?
• A source of content. Where does the data to be displayed come from?
• A source of metadata. How do you annotate the data in ways custom to your application?
• Auxiliary services. What other software does your application need to work with? How will that work?
• Asynchronous task workers. What work does your application do "in the background"?

Writing Good Tests

In general, keep tests located next to the code they cover. So if the model is defined in /apps/classes/models.py, the test should be in /apps/classes/models_test.py.

When a flaky test is discovered, create a ticket immediately for fixing it. Do not let them pile up!

REST API Standards

• The API should be RESTful and fully documented in an auto-generated API Spec file.
• Implementation details should be hidden from the frontend. This allows you to change implementation strategies, do data migrations, and do A/B tests without needing to modify the frontend.
• As much as possible, the frontend code should use models defined by the auto-generated API client rather than re-defining them.
• The API should be versioned, e.g. /api/v1/endpoint/:id not /api/endpoint/:id.

Testing APIs

REST API tests should guarantee API functionality and backwards compatibility. A test failure should represent a breaking change to the REST API contract. Internal refactoring should never result in breaking these tests.

To achieve that:

1. Data setup should happen via REST API. No factories or models.
2. Introspection should happen via REST API. No query counts or model queries.
3. Tests should look for particular fields of interest but not assume they know every field, since additional ones can be added in a backwards compatible way. Testing one attribute at a time also makes it a lot easier to see when there is a regression, instead of trying to look at a large diff.

User IDs

These guidelines from edX require that "there must not be any APIs or pages that expose the association between the LMS user_id and any of the user's PII (e.g. username and full name) to unauthorized users." Since we allow unauthorized/unregistered users to view many pages on the site, including the discussion forums, which are full of usernames and display data about users retrieved from our APIs, that rules out using the LMS user ID and LabXchange user ID in our APIs. Our REST API should use usernames to identify users throughout. This is simple, URL-friendly, and has the nice advantage that the usernames are the same in LabXchange and in the edX LMS (unlike the user IDs).

User Permissions Matrices

A.k.a. How to write code that enforces the rules.

Power rules with Bridgekeeper.

Some guidelines:

1. Avoid checking properties of the user in views etc to determine access. Keeping the checks in a rule which is re-used in multiple places allows easy updates of permissions for different roles as requirements evolve.
2. Rules that are used to determine access should correspond to specific actions. These are rules whose name is prefixed with "can_". Any "is_" rules should only be used in constructing "can_" rules for specific actions.

Developer Experience

When a model is added or modified, provide Django fixtures to go with it.

This helps with unit testing and speeds up manual tests by providing a way for the reviewer to add sample data.

Make sure any references to users or other database objects in your fixture use natural keys, since the user IDs may be different for different developers.

Makefiles for common developer commands are a great idea! Don't make developers remember which obscure incantation will load their test data or provision their devstack.