* refactor: reusable connection error and permission denied alerts
This commit pulls the connection error and permission denied alerts out of ProctoredExamSettings and also makes them Open edX friendly by removing references to “edX” and using the SUPPORT_URL environment variable to supply the support link.
This is in preparation for using these alerts in the Discussions UI.
* refactor: saveAppConfig now responsible for redirect
I’ve moved the redirect to the pages and resources path into the thunk for saveAppConfig. This is because we only want to do it if the thunk is successful, and it’s easier to do it here than to have `then` and `catch` handlers in the component. In particular, this is because we can’t stop the `then` from happening unless we throw an error from the thunk, but the component has nothing to do on a thrown error. This avoids the awkward code in the component and just handles it all here.
* feat: handle access denied by setting DENIED statuses
This takes us one step closer to user messaging for permission denied errors by setting an explicit DENIED status and saveStatus when a request was denied because the user didn’t have permissions. Note that this is different than a 401, which is unauthorized, meaning the user is logged out. This doesn’t handle 401s.
Following this, we can then use the DENIED status to give the user feedback on what’s going on.
* feat: adding error alerts
This commit adds friendly error alerts for connection and permission denied errors in the discussions app.
If the initial fetch apps request errors out or is denied, then the entire contents of the modal is replaced with an error message.
If the save app config request errors out, then a message is displayed at the top of the form.
If the sae app config request is denied, the entire contents of the modal is replaced with a permission denied error message, as in the first case.
* fix: internationalize some strings that got left behind
- General
- Questions for the TAs
* fix: making the questions for TAs field consistent with the general topic one
* fix: improving i18n descriptions as per review feedback
* test: adding @testing-library/user-event
We use this library to simulate user clicks on interactive elements.
* test: adding some test environment mocks
Paragon components tend to use some advanced browser features that aren’t available in node.js/jest’s environment. This safely mocks those features so that testing can proceed.
* test: Make our API responses reusable
Our future tests for DiscussionsSettings will want to use these API responses, so pull them out into a “factories” directory so multiple tests can re-use them.
The directory is called “factories” because ideally we’d use a test data factory to generate this data if we need a number of varieties of it. Right now we just need these two, so generating factories for it isn’t really worthwhile. But we might as well put it in the right place.
* refactor: use new CheckboxControl for AppCards
This replaces our usage of Input for checkboxes with a new CheckboxControl which is made specifically for this sort of use case.
It also adds an aria-label that describes what the checkbox does: “Select <appName>”
* feat: adds aria-label to Next button
The button lacked a label - this also lets us use queryByLabelText in our tests.
* test: favoring getByRole over getByTestId
The testing-library documentation talks about how it’s preferable to write tests that act like screen readers and inspect the DOM in ways that a user would. Adding “test IDs” is a last resort when no better option is available. The spinner actually has a “status” role on it, so we can use that instead here.
* feat: adding a spinner to the AppConfigForm
Improves the UX here by giving the user some feedback, and also makes it consistent with how AppList works.
* fix: set the selected app in redux so it’s official
Prior to this, we derived a selected app from the activeAppId if one wasn’t actively selected, but we never sent that decision back to redux. This closes the loop.
* fix: add the message for selecting an app
Forgot to include this in a prior commit. Oops.
* refactor: use Paragon Stepper and FullscreenModal
Also deletes our app-specific implementations of Stepper and FullScreenModal.
Note that the routes were pulled up into PagesAndResources. This is so that we can access the appId param in DiscussionsSettings, and is an artifact of how react-router works. You can’t access sub-route params in the same component that defines sub-routes.
Related to this, we now decide which step we’re on by examining the appId parameter, rather than having a route per step. Conceptually it’s the same and each step has its own route, but now DiscussionsSettings just has multiple routes and doesn’t define subroutes.
* test: Adding tests proving that DiscussionsSettings works
This exercises the modal and stepper, proving that they interact with routes properly. It also exercises the navigation buttons.
* doc: documenting selectedAppId and activeAppId
* fix: removing unnecessary aria-label attributes
aria-label is only necessary on buttons if the button text doesn’t sufficiently label the button, i.e., in the case that the button text is an “X” instead of the word “Close”. This removes unnecessary button aria-labe attributes and updates the tests not to use them.
* test: adding more DiscussionsSettings tests
- form submission
- loading the ‘legacy’ form
* test: improving coverage for “full support” apps
* fix: PageCard should use the pages and resources path from context
This prevents some odd behaviors around changing the URL based on the current path.
* fix: use the correct formRef for AppConfigForm children
Straight up refactoring bug. This should be using the formRef from the context, but was still defining its own and passing it down. This ultimately meant that the submit button wasn’t properly hooked up to the form, since the form was given a different ref than the submit button.
* fix: Only validate fields that have been touched
* fix: use the title instead of app.name
app.name doesn’t exist - but the title is the string we’re looking for. Use it.
* refactor: default appConfig appropriately for both config forms
In subsequent PRs we’re going to start passing null appConfigs to forms sometimes - this uses prop types to set default values for the forms if the component isn’t passed a meaningful appConfig.
* refactor: replace and flesh out discussions data layer
Since we only have one GET endpoint for all the data around discussions, we don’t need two separate slices/thunks/api files. The API needs to be hit once when the discussions settings load. This means the app-list and app-config-form share far more state than originally thought.
The AppList and AppConfigForm are no longer responsible for data loading - we’ve moved that back up into DiscussionsSettings. Now they just read from the redux state and load what they need.
The main change is moving app-list/data/slice.js up to discussions/data/slice.js, renaming the reducer, and adding a saveStatus to it - turns out this is all we need. The original two slices go away.
The discussions/data/api file gets a proper implementation of postAppConfig which mostly works with the server as of this writing - we have some data shape issues to figure out.
AppConfigForm needed a little help. It now checks whether our data is loaded and selects an app based on its route. This allows us to deep link in and keep the selectedAppId correct.
* Adding a loading spinner to AppList
This is only tangentally related to the current PR, admittedly.
* test: adding some title tests to LegacyConfigForm.test.jsx
Needed to add the title prop, then decided I’d test it.
* test: adding a test suite for discussions/data files
This test exercises the thunks, slice, and api parts of the data layer all at once in ‘real’ scenarios with mocked API requests.
* fix: adjusting discussion selection step 1 message
Removing a duplicate message too.
* fix: adding a dividing line between all sections of the app config forms
The line is its own component because it has some custom styling to make it go edge-to-edge.
There’s a known issue here where a divider inside a TransitionReplace component won’t show its full width until the transition is finished. No idea how to fix that.
* fix: moving app titles inside the config forms.
Also moving the Card styling in too, since it sorta goes with the dividers and the overall look of the specific forms moreso than the parent.
* fix: allowing dividers to be thicker between sections
This defaults our discussions provider to legacy if one was not specified by the backend.
It also fixes a few navigation and provider selection bugs that I encountered in testing the above.
* fix: removing unnecessary CSS classes and files
The .discussion-app-card and .features-table classes aren’t actually used - that means the AppList.scss file and pages-and-resources/index.scss above it can both be deleted.
* refactor: moving AppList components into app-list sub-directory.
* refactor: moving ConfigFormContainer and children into app-config-form sub-directory
* refactor: Renaming ConfigFormContainer to AppConfigForm
Makes it consistent with app-list/AppList.
* refactor: providing discussions path to sub-components via a context
This will be used in subsequent commits as we move functionality out of DiscussionsSettings.
* fix: change step two label to “Settings”
The mockups changed the label, which is great, since we’re aiming to remove the need for selectedAppId to be in DiscussionsSettings. This is one step toward that.
This has the side effect of making app.name no longer necessary, which is great, since the app no longer contains its own name string.
* refactor: adding replacement data layer for app-list and app-config-form
This code is a replacement for the slice, thunks, and api in /discussions/data. It’s an “expand” commit that adds the new implementation. The next commit will use it, and the last will remove the old implementation.
Those files in /discussions/data contained API functions, thunks, and reducer state for two separate components (app-list and app-config-form) that don’t actually share any code. Turns out they’re perfectly separable, and more understandable apart.
This new code isn’t used yet.
* refactor: add a context to provide a formRef to AppConfigForm components
This is an intermediary commit - it’ll likely fail linting since the whitespace is a bit off. It’s also not doing anything yet - a subsequent commit will make use of this code (the provider and new ApplyButton component)
Today the DiscussionsSettings component needs to be responsible for form submission because it owns the “Apply” button that a user clicks to submit the form. It declares a “formRef” which is passed into the form and attached to the form DOM element, and then the “Apply” button can call formRef.current.submit() to submit the form from DiscussionsSettings, even though the form itself is declared way down in a sub-module.
This is awkward, and forces this top level component to be smarter than it should be, and to know too much about its children.
An alternate pattern is to create a context provider (AppConfigFormProvider) which owns the formRef and is part of the sub-module. Then create an AppConfigFormApplyButton component that knows how to pull that formRef out of the context and call submit on it.
This way, DiscussionsSettings doesn’t know anything about form submission, and we can move all that code into the app-config-form sub-module that should know about it. Win!
Again, this commit just adds the code and inserts the provider into DiscussionsSettings - it doesn’t use it all yet.
* refactor: create AppListNextButton that knows how to move to step two of the Stepper
Like the previous commit, this will enable us to factor code out of DiscussionsSettings and into a sub-module where it is a more natural fit.
This AppListNextButton makes use of the `selectedAppId` we’ll be adding to our new app-list reducer in order to know which app has been selected in the list. It can then use that to change the URL route.
In contrast to the previous commit, we use redux here instead of a context. We used context for the formRef because we try to keep complex data types like that out of redux, and also because formRef is not _state_, but just an implementation detail of how we needed to wire everything up in a React app. selectedAppId, on the other hand, is state, and so belongs in redux more naturally.
* refactor: cutting over to using the new discussions reducers
This commit will break everything, since the React components haven’t been updated to use the new reducer shape yet. Subsequent commits will convert app-list and app-config-form over.
* refactor: remove app-list logic from DiscussionsSettings
This is a ‘contract’ commit which removes functionality from DiscussionsSettings and adds it to AppList.
A word of warning that I tried my best to separate this refactor from that of AppConfigForm, but this commit may not be perfect. The original problem was that the two sub-modules were unnecessarily intertwined, making use of some shared variables when they didn’t need to (such as selectedAppId), so I can’t fully get app-list working without touching app-config-form as well… so this particular commit probably has logical errors and won’t build properly. Taken together with the next commit for app-config-form, though, they move a significant amount of logic out of DiscussionsSettings and into the sub-modules and isolate them from each other.
You’ll see here that handleSelectApp and handleStartConfig move into the app-list sub-module. The former goes into AppList, the latter was committed to AppListNextButton in a prior commit.
Once those two are gone, a bunch of other code in DiscussionsSettings starts falling away. selectedAppId is still necessary since the config form handlers are using it, but we’ll see that disappear in the next commit.
* refactor: move app-config-form logic out of DiscussionsSettings
This is the last major commit. It removes a handleApply and handleSubmit from DiscussionsSettings and then cleans up the various variables and imports which are no longer necessary.
handleApply moved into AppConfigFormApplyButton in a prior commit, and handleSubmit has moved into AppConfigForm itself. Now DiscussionsSettings doesn’t know anything about forms or submission, we’ve successfully moved all that code down into sub-modules.
This commit also makes use of AppConfigFormApplyButton.
* refactor: remove old discussions reducers, thunks, and APIs
These three files are now obsolete and have been replaced by a new set of reducers in app-list and app-config-form, specific to those sub-modules specific functions.
This has the side effect of simplifying the naming of the state variables and making it easier to reason which variables are for what sub-module. Prior to this refactor, there were several logical bugs where we were using a confusing combination of “selectedAppId” in local state, “displayedAppId” and “activeAppId” in redux, along with displayedAppConfigId, and it wasn’t quite clear what was for which part of the app, and mis-use was causing some loading and state errors as users navigated around.
The “status” variable in this reducer was also problematic, as we have two separate async requests we’re trying to track the status of - fetchApps and fetchAppConfig. The app was getting confused about what was loaded and what was still loading, which actually caused an infinite loop for me before choosing to refactor (state changes from the two requests were invalidating the state of the other, toggling back and forth between LOADED and LOADING)
* refactor: moving authoring.discussions.heading fix into the right messages file
* feat: adding form validation to LtiConfigForm and LegacyConfigForm
The three fields in LtiConfigForm each get their validation/feedback hooked up properly, and the blackout dates field in LegacyConfigForm now uses a regex-based validator for blackout date strings.
* doc: Improving comments for the blackout dates regex.
* fix: allow no blackout dates
* chore: adding jest-dom package and configuring it
Also bumping version of paragon - it’s in this commit because the changes in package-lock.json can’t really be separated from each other.
* fix: improve mocked data in the API layer
Make the mocked data for the app configs closer to reality, using correct shape and better IDs.
* fix: improve layout of FormSwitchGroup and make compatible wit latest paragon
Form.Group needs a controlId, and this layout gives a nice gutter between the text side on the left and the switch itself on the right.
* fix: active vs. displayed apps and app configs
We have a problem in that the app config and app that are _displayed_ in the frontend are not necessarily the same as app and app config that’s _active_ for the course. I.e., maybe I’m configuring a new one, but a different integration’s already set up.
This commit changes our data model a bit to differentiate between the two - this will let us display information about what’s currently active at the same time as configuring a different integration.
This commit also tweaks a Container size to make the form a bit wider. Pretty.
* refactor: split LegacyConfigForm parts out into their own components
This is in preparation for needing to share legacy config form fields with the ‘standard’ config form for the new discussions MFE. In particular, we also need to pull the InContextDiscussionFields out of the legacy form - that component exists but isn’t technically used in this commit. It will be included in the ‘standard’ form soon.
- This uses a new function in frontend-platform 1.9.0 that sets up a mock application for use in test suites.
- The ProctoredExamSettings.test.jsx tests have been refactored to use axios-mock-adapter and initializeMockApp.
- A tweak was made to ProctoredExamSettings to use error.response.status instead of error.customAttributes.httpErrorStatus - the former is a more canonical way of getting at this data, rather than using the customAttributes object we add to our errors. It also means the code will work with the MockAuthService instead of AxiosJwtAuthService.
- Removing axios dev dependency - we don’t need it anymore!
Prior to this, the list of providers in the UI was hardcoded in the api.js layer.
This commit hooks that up to our actual API endpoint, allowing us to load the provider list from the server.
It also - out of necessity - changes the way the AppCards are displayed, and what content is in them. This was somewhat opportunistic as our design for them simplified anyway, no longer requiring a logo or a few of the other fields.
Because the actual API sends us less display-oriented data (i.e., no names or descriptions for things), we had to modify FeaturesTable and AppCard to fetch these strings from the messages file based on the app and feature IDs.
I’m not super thrilled with this approach, since it’s somewhat brittle. Unexpected providers won’t display properly. In the long run, I expect all this may come from some other system. Using translations to load dynamic strings isn’t quite what it was intended for - i.e., we’re not putting “Piazza” in there because we want to translate it, but because the backend didn’t tell us what to use for the key “piazza”.
* feat: Implement edX forums advanced setting editor
This change implements the UX for the advanced settings editor for the internal edX forums.
* Stepper now intelligently shows/hides drop shadows on the header and footer
Also organized the component into a more Paragon-like organization with the sub-components hanging off the main one.
* Organizing full screen modal in a more Paragon-like way.
Also fixing a few minor styling issues.
* Massaging SettingsEditor into LegacyConfigForm
- Reorganizing into apps/legacy directory from settings/base-forum
- Using Formik and Yup for managing form data
- Refactoring FormSwitchGroup a little and adding data-related properties
- Also moving FormSwitchGroup into ‘generic’
- Some initial attempts at error validation in LegacyConfigForm (a blackout dates regex which doesn’t seem to work yet)
- Sub-sections of config now fold and animate when their parent is toggled.
* Minor naming and refactoring of Discussions component
- Event handlers should be named handle*, not on*. Oops. Got over-zealous.
- Organizing paths into some variables at the top of the component. The pages and resources path should probably be passed in.
* Hooking up and organizing LTI and Legacy forms
- The LTI form moves into app/lti
- Adds in rendering of the legacy discussions form.
- Splits up the messages file a bit (app/lti/messages.js exists now)
- Removing unnecessary h1 in the LtiConfigForm.
* Removing ‘info’ blue coloring from the pages & resources view.
Co-authored-by: Kshitij Sobti <kshitij@sobti.in>
* Bumping paragon version to latest.
* Modifying event handlers to be named with “on” prefix instead of “Handler” suffix
* Tweaking a message id.
* Removing “Discussion” prefix from discussions components.
Seems unnecessary.
* Backing pages & resources view with data handling.
It still has the list of pages hard coded.
* Adding FullScreenModal and Stepper components.
These components are pretty close to their final form. They could benefit from some snapshot tests and such; there isn’t much actual functionality in ‘em.
Stepper will get a bit more functionality when we add the dynamic drop shadow behavior. Depending on whether the stepper body is at the top or bottom, drop shadows on the header and footer should appear or disappear to indicate more content exists above or below the viewport.
* Moving discussions routes inside PagesAndResources
Note that the discussions component has been renamed - that’ll be coming in a subsequent commit.
Also trying to get consistent about calling it “discussions”
* AppList gets less responsibility
The AppList is now a child of the top-level “Discussions” component, so it’s no longer responsible for loading the app list or storing the state for the selected app ID. It’s also given a handler for when an app is selected, and no longer has a button to configure the app.
* Fleshing out Discussions component
The top-level Discussions component (renamed from DiscussionsRoutes) is now responsible for a lot.
- it loads the app list
- it keeps track of selected app ID
- it has handlers for all the various user actions so they can be coordinated here at the top.
- it uses component composition to create the majority of the UI, folding together FullScreenModal and Stepper with its route-based views.
* Decomposing the app config form
The discussion app config form has been decomposed into a container responsible for loading app data, and a component specifically for the LTI configuration form.
In the future, ConfigFormContainer will get a second possible child for the edX Forums app, and will switch between the two forms based on the app being configured.
Note that I expect that some of the data loading logic from ConfigFormContainer may be better situated in the Discussions component… everything else is happening there, and it may make sense for it to handle loading the app config data as necessary as well.
