-
Notifications
You must be signed in to change notification settings - Fork 99
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
Question duplication #1423
Question duplication #1423
Conversation
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.
Hey @KaasKop97,
nice start, thank you! :)
Got a few comments here, apart from that, can you please add the route to the API docs then?
Greets,
Jonas
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1423 +/- ##
=========================================
Coverage 44.59% 44.59%
- Complexity 653 657 +4
=========================================
Files 58 58
Lines 2563 2592 +29
=========================================
+ Hits 1143 1156 +13
- Misses 1420 1436 +16 |
I've applied almost all of your proposed changes except for
In the ApiController file, this is because the id is required to be there for QuestionMultiple if it's not present you'll receive errors in the javascript console. Thanks for the thorough review though! I am a bit rusty on php and javascript :) |
Yes, you need to return the question including ids. But that also works, if you just read the
|
5d1fb2e
to
7276ce4
Compare
Thank you again for this work, I took the liberty to squash your commits and pushed a commit fixing the remaining review comments. |
@susnux we need to add the route to the docs as Jonas already mentioned :) |
ed59ef9
to
5421000
Compare
Yes this is resolved |
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.
Looks good in general :) Should we add a "Copy of " to the question title or something like that?
I would say this does not make much sense for questions and would make the workflow more complex? |
* ApiController can now receive a duplication request, copies the question and options to new ones and then returns that new question object. * All questions can now handle duplication. * Create can now handle the duplication of questions. * Added the new api route. * Some styling and variables renamed to fit nextcloud guidelines * Written an integration test. * Added some comments to new methods added. * Added start for translation * Refactored variable names and some cleanup. * Create is now more concise. * Updated routes Signed-off-by: Mitchel van Hamburg <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux Squash the commits into one and then we're good to go :) |
This implements the feature of #595.
Signed-off-by: Mitchel van Hamburg [email protected]