search

lakesare / memcode

Spaced-repetition: with real formatting.
http://memcode.com
MIT License
339 stars 73 forks source link

Print all cards option #151

Closed piotrvidal closed 2 years ago

piotrvidal commented 2 years ago

A new option for printing all cards on desk was added. I also refactored the existing select in CourseUserIsLearningModel stuff to use knex instead of the old way. The rest of stuff in CourseUserIsLearningModel is still un-knexed (it doesn't seem to be used anywhere, though).

However, having two separate routes for each option results in a bit of code duplication. I'd suggest refactoring it to use only one route /courses/:id/print and send a boolean in the request to decide whether to retrieve only review or all flashcards. Any other suggestions appreciated.

kgashok commented 2 years ago

Looking forward to having this functionality, soon! @piotrvidal

lakesare commented 2 years ago

CourseUserIsLearningModel is still un-knexed (it doesn't seem to be used anywhere, though)

Feel free to delete stuff from /models when it's not used anywhere. I'm trying to get rid of the /models folder, now that we use knex it's easier to query database directly in /api files.

However, having two separate routes for each option results in a bit of code duplication. I'd suggest refactoring it to use only one route /courses/:id/print and send a boolean in the request to decide whether to retrieve only review or all flashcards. Any other suggestions appreciated.

This is exactly what we should do, see how we do it with /courses/:id/review/simulated and /courses/:id/review/persistent.

I will merge it right away however to avoid merge conflicts down the line, and to enable the functionality, we can refactor it later (feel free to create the refactoring PR!).