Skip to content

Stevenwaldron01#4

Open
stevenwaldron wants to merge 3 commits intomasterfrom
stevenwaldron01
Open

Stevenwaldron01#4
stevenwaldron wants to merge 3 commits intomasterfrom
stevenwaldron01

Conversation

@stevenwaldron
Copy link
Copy Markdown
Collaborator

No description provided.

@takakonishimura takakonishimura force-pushed the master branch 4 times, most recently from 1bda05e to 7e697a6 Compare September 10, 2020 22:10
@takakonishimura
Copy link
Copy Markdown
Contributor

takakonishimura commented Sep 27, 2020

@stevenwaldron I like how thorough your implementation of the models, serializers, and viewsets is.

Please do the following to integrate it into our engine:

  • Pull all changes from origin master and then rebase the updated master branch as per our PR standards
  • Since the project was renamed, you will likely need to create a new app inside engine (the new project name) called curriculum.
  • It may be better to work off of master and add everything you have back given the master branch has a .gitignore file in it that would prevent ou from including all of the .pyc, bin/ and other files which should not be part of the repository.

Design Feedback
I think there are parts of the spec which are not implemented:

  • All APIs exposed to the frontend need to be expose through the api app and routed to your subapp (example)
  • I'd like you to add the relations between curriculum and the modules as well as the modules and their submodules as described here

I will review the database models themselves after the above two sections are done, but here are initial feedbacks I see for the API and datamodel:

  • Retrieving a curriculum should imply retrieving all of the associated module ids. Similarly, retrieving a module should imply retrieving all of the submodule ids.
  • Submodules submodule types should be defined as enums as listed here, then the corresponding table would be queried, however lesson article and quiz do not need their own tables as the data is grouped into folders (lesson data, quiz data)
  • Creating a module should require you to state what curriculum it will be a part of. Same for a submodule.
  • There wouldn't be a case where we would want to retrieve all articles, all quizzes, or all lectures so those paths can be removed.
  • We need to discuss better what deleting means for all of these objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants