-
Notifications
You must be signed in to change notification settings - Fork 0
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
Django Creator Part 2 #33
Conversation
…o django-api-split
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.
Approved, with the provision that there are some changes to things that will still need to be adjusted or deleted in the future: some examples include changes to the JS API util that are on deprecated/NYI endpoints and the Question model.
src/components/widget-creator.jsx
Outdated
inst.qset.data, | ||
inst.qset.version | ||
]) | ||
apiGetQuestionSet(inst.id).then((qset) => { |
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.
Pairing the qset with widget instance data in this context slipped past me - but I think I'm generally OK with having a dedicated request for it here. Keeping these resources separated feels cleaner, even if it means an additional hit on the API. I'm open to being convinced otherwise though.
|
||
# Mimics PHP's FILTER_VALIDATE_BOOL | ||
@staticmethod | ||
def validate_bool(raw_bool: str, default: bool = False) -> bool: |
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.
I think I'm OK with this method existing, but anywhere that isn't using explicit boolean values should be judiciously updated to only use boolean values. One of the hard-line goals of the django rewrite is to eliminate any future uses of nonsense like foo == "1"
for boolean checks.
@@ -579,12 +579,21 @@ class Question(models.Model): | |||
type = models.CharField(max_length=255) # type is a "soft" reserved word in Python | |||
text = models.TextField() | |||
created_at = models.DateTimeField(default=datetime.now) | |||
data = models.TextField(blank=True, null=True) | |||
_data = models.TextField(blank=True, null=True, db_column="data") |
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.
These changes may be redundant if we choose to retire the Question model, but I don't see the harm in including them preemptively.
Adds many of the remaining features for the creator. Additionally introduces and changes some util classes, as well as a lot of cleanup. Completes much of the remaining tasks in #20 (with some exceptions)
generator_util.py
util class. Generation config is loaded in through a.env
file that has the same format as the config for Materia PHPopenai
package torequirements.txt
for this functionalityWidgetInstanceUtil
methods.js_global_vars
now properly supports settingNone
,str
,bool
, and numbers as global JS variables appropriately (whereas before every item was just pasted in as a JS string object).MsgUtil
class intoMsgBuilder
andMsg
.Msg
now exists as a standalone class that can be used to pass error states internally.Msg
objects have.as_json_response()
and.as_drf_response()
to easily convert them to formats that can be sent to the client.MsgBuilder
is a factory class that helps in creating many of the common message types, such asno_login
orfailure
.ContextUtil
class and ported all existing context creations to use it. Eases the creation of Context objects, largely by auto-filling many of the required or almost-always-required fields automatically.UserUtil
class, largely to provide averify_session
function that acts similarly to the version present in PHPThings that are not included in this branch: