Clean up workshop dashboard: make proper js entry point, remove globa… #21689
Conversation
…ls, add shared constants, add redux
| componentWillMount() { | ||
| if (this.props.showSurveyUrl) { | ||
| this.permission = new Permission(); | ||
| constructor(props) { |
There was a problem hiding this comment.
👍 Nice cleanup of to-be-deprecated lifecycle method.
| if (this.props.showSurveyUrl) { | ||
| this.permission = new Permission(); | ||
| constructor(props) { | ||
| super(props); |
There was a problem hiding this comment.
What's this for? Does the base class have any constuctor logic?
There was a problem hiding this comment.
Yes, this is required because the base React.Component expects props in the constructor. This is a common React pattern. See https://reactjs.org/docs/state-and-lifecycle.html#adding-local-state-to-a-class
| } | ||
| }; | ||
| }; | ||
|
|
||
| it("Detects workshop admin", () => { |
There was a problem hiding this comment.
I feel like all of this is DRY-able.
permission_types = ['WorkshopAdmin', 'Facilitator', 'Organizer', 'ProgramManager', 'Partner'];
permission_types.each ((type) => {
const permission = new Permission([type])
expect(permission.has(type)).to.be.true;
permission_types - type.each((other_type) => {
expect(permission.hash(other_type)).to.be.false;
})
Something along those lines
There was a problem hiding this comment.
Agreed, but I didn't change this test, just got it to work with the new format. I'd like to clean it up in a future PR since this one is already large.
| @@ -0,0 +1,65 @@ | |||
| module Pd | |||
There was a problem hiding this comment.
Is this generated from the other worshop_constants file? If so, should this be checked in?
There was a problem hiding this comment.
No, this is the source of the shared constants. It's included in Pd::WorkshopConstants, which also contains non-shared Ruby-only constants, and it generates a javascript /apps/src/generated/pd/sharedWorkshopConstants.js file which is not checked in. In fact everything under apps/src/generated is gitignored.
|
@islemaster yeah sorry this got a bit larger than I initially intended. I could have split it up into a few smaller PRs. Thanks for diving in! The permissions were earlier provided globally on the |
…ls, add shared constants, add redux.
I also cleaned up the client-side Permission class slightly and moved it into WorkshopDashboard since that's the only place it's used.
This has been a long tech debt item that I chose now as a prerequisite to filtering which courses facilitators have access to when creating workshops (#21690). That feature, which requires passing new data (the allowed courses) from server to client, is much simpler with a cleaner entry point and redux.
It might be easier to review with whitespace ignored: https://github.com/code-dot-org/code-dot-org/pull/21689/files?w=1
The Ruby workshop constants are unchanged, just moved into two new modules (
Pd::SharedWorkshopConstantswhich is shared with JS, andPd::WorkshopConstantsfor Ruby only, which includes the former).