-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move main Scorer and Validator to PerseusScore #2165
Changes from 14 commits
8aa4028
b25fb6c
d2387fc
3523c8f
8fc194e
15c03e5
39efebe
0934f34
f764d86
5348013
14450db
38ea7b6
de0e4ae
e894520
7d73db2
f231ad4
4216e8a
d7f2607
83080a4
34055a0
19a5d84
d9a6699
488f663
a916f59
852109c
8fed47e
47569f0
de42db3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ export type { | |
MarkerType, | ||
InteractiveMarkerType, | ||
Relationship, | ||
Alignment, | ||
} from "./types"; | ||
export type {ErrorKind} from "./error/errors"; | ||
export type {FunctionTypeMappingKeys} from "./utils/grapher-util"; | ||
|
@@ -110,8 +111,15 @@ export type {TableDefaultWidgetOptions} from "./widgets/table"; | |
export {default as videoLogic} from "./widgets/video"; | ||
export type {VideoDefaultWidgetOptions} from "./widgets/video"; | ||
|
||
export { | ||
getUpgradedWidgetOptions, | ||
upgradeWidgetInfoToLatestVersion, | ||
} from "./widgets/upgrade"; | ||
|
||
export type * from "./widgets/logic-export.types"; | ||
|
||
export * as WidgetLogic from "./widgets/widget-registry"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming difference here feels like perhaps this should be exported as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I renamed the export and file to |
||
|
||
export {default as getOrdererPublicWidgetOptions} from "./widgets/orderer/orderer-util"; | ||
export {default as getCategorizerPublicWidgetOptions} from "./widgets/categorizer/categorizer-util"; | ||
export {default as getExpressionPublicWidgetOptions} from "./widgets/expression/expression-util"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,9 +10,9 @@ const defaultWidgetOptions: GradedGroupSetDefaultWidgetOptions = { | |
gradedGroups: [], | ||
}; | ||
|
||
const GradedGroupSetWidgetLogic: WidgetLogic = { | ||
const gradedGroupSetWidgetLogic: WidgetLogic = { | ||
name: "graded-group-set", | ||
defaultWidgetOptions, | ||
}; | ||
|
||
export default GradedGroupSetWidgetLogic; | ||
export default gradedGroupSetWidgetLogic; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This were just mistakes from previous PRs. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ const defaultWidgetOptions: ImageDefaultWidgetOptions = { | |
const imageWidgetLogic: WidgetLogic = { | ||
name: "image", | ||
defaultWidgetOptions, | ||
supportedAlignments: ["block", "full-width"], | ||
defaultAlignment: "block", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alignment is used during the upgrade process, that's why I moved all of these. |
||
}; | ||
|
||
export default imageWidgetLogic; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,158 @@ | ||||||||||||||||||||||||||||||||||||||||||||
import _ from "underscore"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
import {Errors} from "../error/errors"; | ||||||||||||||||||||||||||||||||||||||||||||
import {PerseusError} from "../error/perseus-error"; | ||||||||||||||||||||||||||||||||||||||||||||
import {mapObject} from "../utils/objective_"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||
getCurrentVersion, | ||||||||||||||||||||||||||||||||||||||||||||
getDefaultWidgetOptions, | ||||||||||||||||||||||||||||||||||||||||||||
getSupportedAlignments, | ||||||||||||||||||||||||||||||||||||||||||||
getWidgetOptionsUpgrades, | ||||||||||||||||||||||||||||||||||||||||||||
isWidgetRegistered, | ||||||||||||||||||||||||||||||||||||||||||||
} from "./widget-registry"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
import type {PerseusWidget, PerseusWidgetsMap} from "../data-schema"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const DEFAULT_STATIC = false; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
export const upgradeWidgetInfoToLatestVersion = ( | ||||||||||||||||||||||||||||||||||||||||||||
oldWidgetInfo: PerseusWidget, | ||||||||||||||||||||||||||||||||||||||||||||
): PerseusWidget => { | ||||||||||||||||||||||||||||||||||||||||||||
const type = oldWidgetInfo.type; | ||||||||||||||||||||||||||||||||||||||||||||
// NOTE(jeremy): This looks like it could be replaced by fixing types so | ||||||||||||||||||||||||||||||||||||||||||||
// that `type` is non-optional. But we're seeing this in Sentry today so I | ||||||||||||||||||||||||||||||||||||||||||||
// suspect we have legacy data (potentially unpublished) and we should | ||||||||||||||||||||||||||||||||||||||||||||
// figure that out before depending solely on types. | ||||||||||||||||||||||||||||||||||||||||||||
if (!_.isString(type)) { | ||||||||||||||||||||||||||||||||||||||||||||
throw new PerseusError( | ||||||||||||||||||||||||||||||||||||||||||||
"widget type must be a string, but was: " + type, | ||||||||||||||||||||||||||||||||||||||||||||
Errors.Internal, | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!isWidgetRegistered(type)) { | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change, basically just adding a helper here. |
||||||||||||||||||||||||||||||||||||||||||||
// If we have a widget that isn't registered, we can't upgrade it | ||||||||||||||||||||||||||||||||||||||||||||
// TODO(aria): Figure out what the best thing to do here would be | ||||||||||||||||||||||||||||||||||||||||||||
return oldWidgetInfo; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Unversioned widgets (pre-July 2014) are all implicitly 0.0 | ||||||||||||||||||||||||||||||||||||||||||||
const initialVersion = oldWidgetInfo.version || {major: 0, minor: 0}; | ||||||||||||||||||||||||||||||||||||||||||||
const latestVersion = getCurrentVersion(type); | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change, basically just adding a helper here. |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// If the widget version is later than what we understand (major | ||||||||||||||||||||||||||||||||||||||||||||
// version is higher than latest, or major versions are equal and minor | ||||||||||||||||||||||||||||||||||||||||||||
// version is higher than latest), don't perform any upgrades. | ||||||||||||||||||||||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||||||||||||||||||||||
initialVersion.major > latestVersion.major || | ||||||||||||||||||||||||||||||||||||||||||||
(initialVersion.major === latestVersion.major && | ||||||||||||||||||||||||||||||||||||||||||||
initialVersion.minor > latestVersion.minor) | ||||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||||
return oldWidgetInfo; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// We do a clone here so that it's safe to mutate the input parameter | ||||||||||||||||||||||||||||||||||||||||||||
// in propUpgrades functions (which I will probably accidentally do at | ||||||||||||||||||||||||||||||||||||||||||||
// some point, and we would like to not break when that happens). | ||||||||||||||||||||||||||||||||||||||||||||
let newEditorOptions = _.clone(oldWidgetInfo.options) || {}; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const upgradePropsMap = getWidgetOptionsUpgrades(type); | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change, basically just adding a helper here. |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Empty props usually mean a newly created widget by the editor, | ||||||||||||||||||||||||||||||||||||||||||||
// and are always considerered up-to-date. | ||||||||||||||||||||||||||||||||||||||||||||
// Mostly, we'd rather not run upgrade functions on props that are | ||||||||||||||||||||||||||||||||||||||||||||
// not complete. | ||||||||||||||||||||||||||||||||||||||||||||
if (_.keys(newEditorOptions).length !== 0) { | ||||||||||||||||||||||||||||||||||||||||||||
// We loop through all the versions after the current version of | ||||||||||||||||||||||||||||||||||||||||||||
// the loaded widget, up to and including the latest version of the | ||||||||||||||||||||||||||||||||||||||||||||
// loaded widget, and run the upgrade function to bring our loaded | ||||||||||||||||||||||||||||||||||||||||||||
// widget's props up to that version. | ||||||||||||||||||||||||||||||||||||||||||||
// There is a little subtlety here in that we call | ||||||||||||||||||||||||||||||||||||||||||||
// upgradePropsMap[1] to upgrade *to* version 1, | ||||||||||||||||||||||||||||||||||||||||||||
// (not from version 1). | ||||||||||||||||||||||||||||||||||||||||||||
for ( | ||||||||||||||||||||||||||||||||||||||||||||
let nextVersion = initialVersion.major + 1; | ||||||||||||||||||||||||||||||||||||||||||||
nextVersion <= latestVersion.major; | ||||||||||||||||||||||||||||||||||||||||||||
nextVersion++ | ||||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||||
if (upgradePropsMap[String(nextVersion)]) { | ||||||||||||||||||||||||||||||||||||||||||||
newEditorOptions = | ||||||||||||||||||||||||||||||||||||||||||||
upgradePropsMap[String(nextVersion)](newEditorOptions); | ||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||
throw new PerseusError( | ||||||||||||||||||||||||||||||||||||||||||||
"No upgrade found for widget. Cannot render.", | ||||||||||||||||||||||||||||||||||||||||||||
Errors.Internal, | ||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||
metadata: { | ||||||||||||||||||||||||||||||||||||||||||||
type, | ||||||||||||||||||||||||||||||||||||||||||||
fromMajorVersion: nextVersion - 1, | ||||||||||||||||||||||||||||||||||||||||||||
toMajorVersion: nextVersion, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's an important change. Originally we just
So that's why I did what I did. Very open to ideas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's reasonable to throw here. Perhaps we could include information from the widget options that might help us trace which content is causing this? Maybe just add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Minor version upgrades (eg. new optional props) don't have | ||||||||||||||||||||||||||||||||||||||||||||
// transform functions. Instead, we fill in the new props with their | ||||||||||||||||||||||||||||||||||||||||||||
// defaults. | ||||||||||||||||||||||||||||||||||||||||||||
const defaultOptions = getDefaultWidgetOptions(type); | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change, basically just adding a helper here. |
||||||||||||||||||||||||||||||||||||||||||||
newEditorOptions = { | ||||||||||||||||||||||||||||||||||||||||||||
...defaultOptions, | ||||||||||||||||||||||||||||||||||||||||||||
...newEditorOptions, | ||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
let alignment = oldWidgetInfo.alignment; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Widgets that support multiple alignments will "lock in" the | ||||||||||||||||||||||||||||||||||||||||||||
// alignment to the alignment that would be listed first in the | ||||||||||||||||||||||||||||||||||||||||||||
// select box. If the widget only supports one alignment, the | ||||||||||||||||||||||||||||||||||||||||||||
// alignment value will likely just end up as "default". | ||||||||||||||||||||||||||||||||||||||||||||
if (alignment == null || alignment === "default") { | ||||||||||||||||||||||||||||||||||||||||||||
alignment = getSupportedAlignments(type)[0]; | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change, basically just adding a helper here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! The array access makes me a bit paranoid. Could we add a safety mechanism in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add some safety measures and also now throw if somehow there's still not alignment. |
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
let widgetStatic = oldWidgetInfo.static; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (widgetStatic == null) { | ||||||||||||||||||||||||||||||||||||||||||||
widgetStatic = DEFAULT_STATIC; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
return _.extend({}, oldWidgetInfo, { | ||||||||||||||||||||||||||||||||||||||||||||
// maintain other info, like type | ||||||||||||||||||||||||||||||||||||||||||||
// After upgrading we guarantee that the version is up-to-date | ||||||||||||||||||||||||||||||||||||||||||||
version: latestVersion, | ||||||||||||||||||||||||||||||||||||||||||||
// Default graded to true (so null/undefined becomes true): | ||||||||||||||||||||||||||||||||||||||||||||
graded: oldWidgetInfo.graded != null ? oldWidgetInfo.graded : true, | ||||||||||||||||||||||||||||||||||||||||||||
alignment: alignment, | ||||||||||||||||||||||||||||||||||||||||||||
static: widgetStatic, | ||||||||||||||||||||||||||||||||||||||||||||
options: newEditorOptions, | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to the changes you're making, but this feels like a really easy improvement:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, but I ran into a hairy type issue so I had to cast as |
||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
export function getUpgradedWidgetOptions( | ||||||||||||||||||||||||||||||||||||||||||||
oldWidgetOptions: PerseusWidgetsMap, | ||||||||||||||||||||||||||||||||||||||||||||
): PerseusWidgetsMap { | ||||||||||||||||||||||||||||||||||||||||||||
return mapObject(oldWidgetOptions, (widgetInfo, widgetId) => { | ||||||||||||||||||||||||||||||||||||||||||||
if (!widgetInfo.type || !widgetInfo.alignment) { | ||||||||||||||||||||||||||||||||||||||||||||
const newValues: Record<string, any> = {}; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!widgetInfo.type) { | ||||||||||||||||||||||||||||||||||||||||||||
// TODO: why does widget have no type? | ||||||||||||||||||||||||||||||||||||||||||||
// We don't want to derive type from widget ID | ||||||||||||||||||||||||||||||||||||||||||||
// see: LEMS-1845 | ||||||||||||||||||||||||||||||||||||||||||||
newValues.type = widgetId.split(" ")[0]; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+152
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be tempted to start throwing in the face of a missing widget type. Right now, we limp along and gloss over this data this is quite broken (and I suspect oooooold). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this change, but I wasn't sure if you were asking me to or not. I personally would like to avoid making this PR more risky than it already is, but I don't feel strongly about it either way. I agree that it seems low risk to throw here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was ambiguous. Let's not throw for now. |
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!widgetInfo.alignment) { | ||||||||||||||||||||||||||||||||||||||||||||
newValues.alignment = "default"; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
widgetInfo = {...widgetInfo, ...newValues}; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
return upgradeWidgetInfoToLatestVersion(widgetInfo) as any; | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all validators and scorers should be sending error codes instead of error messages, so we don't need strings anymore.