[D7] Course Admin [#2808203]

Project page: https://www.drupal.org/sandbox/webservant316/2808195
Git: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/webservant316/2808195.git course_admin
webservant316 Drupal participation: https://www.drupal.org/user/586896/track

Extension to the 'course' module. Development prototype! Export, Import, Outline, and Delete Entire Courses. The module works in my use case, but please test and use at your own risk.

INTALLATION:
1. The module introduces NO database structure changes.
2. Drupal, 'Course', and other APIs are used to create Course Objects and Content.
3. The module is general purpose and works with many Content TYPES.
4. Enable 'Course Admin' at /admin/modules
5. Configure permissions at /admin/people/permissions#module-course
6. Execute functions at /admin/course/import and /node/COURSE-NID/admin

EXPORT:
Export a ZIP Course Archive for archival or transfer to another LMS. Allows Course Authors and LMS Admins to Export / Import individual courses. The structure of all related Course Content and Field TYPES must remain identical between the Course Export and Import function. Note that only files and images referenced as fields are exported. Files and images referenced within body text are not exported. For example images or files accessed through IMCE are not exported.

IMPORT:
Import a ZIP Course Archive previously exported from Course Admin Export. Allows Course Authors and LMS Admins to Export / Import individual courses.

OUTLINE:
Import a TAB delimited Course Outline with Content TYPE, TITLE, and optional BODY. Helps Course designers create Outlines more easily with tools outside of Drupal. This feature allows Course Designers to 'stub out' an entire course and then visit each Course Page to add their content, text, and fields.

DELETE:
Delete the entire Course, Course Outline, and Course Content. Optionally do not delete course Book and Quiz Content.

REFLECTIONS:
The 'course_admin' module prototype has proved that these functions are possible. This module has also shown that there is much work to do to move modules toward unified Entity handling and APIs. For example Course Export and Import functions would be more easily handled if all node TYPES could be simply copied...

$node = node_load($nid);
$node->nid = 0;
$node->is_new = TRUE;
node_save($node);

However, more complicated nodes TYPES are not prepared for this simplicity. The node_clone() module is used to help properly populate the node for export. So 'webform' node TYPES worked great with this method. However, other node TYPES such as H5P and Quiz Scale nodes need custom consideration.

This patch is needed for Quiz Scale nodes, https://www.drupal.org/node/2802605.
This code was needed to add quiz questions, https://www.drupal.org/node/1892278.
This issue questions about H5P cloning, https://www.drupal.org/node/2799581.

Also 'node_clone' effectively clones nodes while relying on the immediate presence of all node resources such as files and images. However, these resources are not encapsualted into the $node object itself. So extra work
is needed to consider these external resources as well. This is also true for any resource that is only attached to the $node by reference such as entity_references, field_collections, and IMCE links in node body text. The list of complications is so large it is a good question whether such an effort really has a future. I only pressed this far toward a solution because my company and use case required it. I handled the external resources for the content types in my use case by first loading the $node, then assigning my own $node->eternal_resource_variable(s) loaded with the external resources. The $node is then serialized and exported. Upon import the $node is then unserialized and the $node->external_resource_variable(s) is unpacked and saved with the new $node via custom code. In the case of H5P nodes I create an entire Zip Archive of /h5p/content/nid and store it on the $node. It works great, but there is no standard API to handle this complicated process.

Any future for this module should consider modules like 'entity', 'entity_clone', and 'entity_relationship'. Future versions of Drupal will also likely tighten the Entity API to make entity exporting easier, hopefully including all the $node resources. The $node could then be easily imported as a new entity or a revision of the original. Even if there is a future with Drupal Entities, contributed modules may or may not participate voluntarily. Of course future versions of Drupal could force participation through a more restrictive API. But will the Entity API handle all the needs of more complicated Entities such as H5P nodes? Also how will the Entity API handle referenced resources that are too large for easy export and import? For example a $node could reference very large files.

Hopefully this prototype helps Drupal visionaries to understand the need and craft better solutions for the future.

Comments

Comment #1

28 September 2016 at 19:26

webservant316 created an issue. See original summary.

Comment #2

PA robot commented 28 September 2016 at 22:23

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Comment #3

visabhishek commented 5 October 2016 at 18:42

Status:	Needs review	» Needs work
Issue tags:		+PAreview: security

Automated Review

Please fix errors reported on http://pareview.sh/pareview/httpsgitdrupalorgsandboxwebservant3162808195git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

No: List of security issues identified.]

(*) Please sanitize/validate the arg() value before using them

ex : course_admin_delete(arg(1))
course_admin_export(arg(1),$form_state['values']['export_print'],$form_state['values']['export_debug'])

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

Please use inc files to categories the function, Its better to use only hooks in module file
Please format the code as per drupal standard, So its easy to understand.
Please remove all used variable in hook_uninstall

Ex :
variable_get('book_made_simple_forbid_main_page_creation','')

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Comment #4

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 6 October 2016 at 07:35

@visabhishek: user input should only be sanitized for XSS when it is printed to HTML. So using arg() directly is fine, sanitization should only happen if the value is later printed to HTML.

Comment #5

visabhishek commented 6 October 2016 at 10:00

@klausi : Thanks, I will take care in next projects.

Comment #6

webservant316 commented 6 October 2016 at 11:58

Thanks for the comments. I hope to update the module and post back.

Comment #7

PA robot commented 3 November 2016 at 17:25

Status:

Needs work

» Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Comment #8

webservant316 commented 3 November 2016 at 18:46

Okay still interested, but busy on another project. I'll get back in time.

Comment #9

webservant316 commented 28 November 2016 at 20:58

Status:

Closed (won't fix)

» Active

Re-opening request to move this project out of the sandbox.

1. README.txt format fixed to Drupal standards.
2. Secure code issues, use of arg(1) NOT an issue as explained in comments above.
3. inc files used as requested.
4. code format fixed to Drupal standards with grammar_parser.
5. This module creates NO variable data, but only references variables from other modules.

One last thing. Notes above say that I am not following guidelines for master branch.

Could I get some help on the last point. What do I need to do? I am using TortoiseGIT.

Comment #10

webservant316 commented 11 January 2017 at 19:13

Hey I need help with properly setting the master branch of my project. I am using TortoiseGit on Windows. so what do I need to do?

Comment #11

webservant316 commented 17 January 2017 at 20:41

Okay the repository is fixed as well as all the code issues listed. Please review again. Thanks!

Comment #12

webservant316 commented 25 January 2017 at 13:52

Posting back after a week, still hoping for a review.

Comment #13

webservant316 commented 8 February 2017 at 19:41

okay still hoping for a review. please.

Comment #14

webservant316 commented 8 March 2017 at 20:56

Further improvement of code comments to Drupal standards committed.

Comment #15

Drupal8 commented 11 March 2017 at 10:30

Hi you should solve the issue reported https://pareview.sh/node/1349

FILE: /root/repos/pareviewsh/pareview_temp/course_admin.inc
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
2014 | WARNING | Possible SQL injection in db_query() via variable $node
--------------------------------------------------------------------------

This is better to use Drupal PDO.

Comment #16

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 11 March 2017 at 16:53

Status:

Active

» Needs work

@Drupal8: looks like you forgot to change the status. Using placeholders with db_query() is mandatory for security reasons before this can be approved.

Comment #17

webservant316 commented 13 March 2017 at 14:35

Thanks for this catch.

Placeholders are now used with all usage of db_query().

Comment #18

webservant316 commented 13 March 2017 at 14:35

Status:

Needs work

» Needs review

Comment #19

webservant316 commented 11 May 2017 at 20:33

@cigotete reports

Just for comment that I found that under course_admin.info there is a line that seems is not necessary (there is no reference under .module or .inc files): files[] = custom_learnh2o.module.

Thank you, that is fixed.

Comment #20

webservant316 commented 24 May 2017 at 15:31

Further cross-site scripting prevention implement and committed. Do not call t() with variables unless sanitized by filter_xss().

Comment #21

Cyclonecode commented 6 July 2017 at 13:54

Automated review

Pareview reports alot of warnings that you should consider fixing: https://pareview.sh/node/1349

Manual review

Individual user account
Yes: Follows
No duplication
Yes: Does not cause
Master Branch
Yes: Follows
Licensing
Yes: Follows
3rd party assets/code
Yes: Follows
README.txt/README.md
Yes: Follows
Code long/complex enough for review
Yes: Follows
Secure code
Yes: Follows
Coding style & Drupal API usage
(*) Major finding, needs work
(+) Release blocker
Rekommendations:
1. You might consider using drupals string functions instead of strtolower, strlen and so, since the drupal versions adds multibyte support.
2. I would keep any global and static variables used at the top of each function.
3. Consider using drupal_dirname() over dirname().
4. You should really spend some time adding useful comments to your functions.
5. Not really sure why you implemented your own function to check if a module exists i.e _course_admin_module_exists ? Why not simply use module_exists() ?
6. I would add an install file and remove any variables created by the module in a course_admin_uninstall() hook.

This review uses the Project Application Review Template.

Comment #22

webservant316 commented 6 July 2017 at 14:37

Thanks! I will make these fixes.

Comment #23

Aaron23 commented 11 July 2017 at 10:47

Hi ,

the pa review runs your old code for course module i,e, Git: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/webservant316/2808195.git course_admin

Can you change that to latest version control. so that we can make coding standard and reduce errors and warnings

Thanks

Comment #24

webservant316 commented 11 July 2017 at 12:35

ok how do I fix that?

Comment #25

Cyclonecode commented 29 July 2017 at 15:04

@Aaron23 - You are saying that PA review runs the old code? I'm not really sure what you mean with this statement. The PA review runs on the branch 7.x-1.x and which should be the development branch for this module?

Comment #26

Aaron23 commented 31 July 2017 at 07:06

Can you check this link https://pareview.sh/node/2532 I ran this git url " https://git.drupal.org/project/course.git "

Thanks,
Aaron23

Comment #27

webservant316 commented 31 July 2017 at 11:37

this is project/course_admin, NOT project/course

Comment #28

Aaron23 commented 31 July 2017 at 12:52

Oops a big sorry for the confusion. Yes its course_admin.

https://pareview.sh/node/2295 Please check this and fix all the bugs

Thanks,
Aaron

Comment #29

Aaron23 commented 31 July 2017 at 13:11

I'm Unable to install this module

I enabled course 7.x-1.11

When I try to install course_admin module.

Sub modules missing. It says,

Clone (missing)

Thanks
aaron23

Comment #30

webservant316 commented 31 July 2017 at 13:10

okay will do, thanks.

Comment #31

webservant316 commented 31 July 2017 at 13:22

node_clone is a dependency. The dependency has to be specified as 'clone' even though the dependent module is named 'node_clone'. This is because the node_clone module is mis-named as clone.module. I will improve the readme to make that clear.

Comment #32

webservant316 commented 10 April 2018 at 17:24

Per #28 and @Aaron23 all the warnings and errors have been removed as referenced at https://pareview.sh/pareview/https-git.drupal.org-sandbox-webservant316-.... There is one warning remaining saying that test scripts would be useful, but not required. Those may be added later.

Hoping that this project can be moved out of the sandbox and it is working wonderfully in production in my use case. Please check the module documentation for further insights into the value of this project.

Comment #33

webservant316 commented 19 July 2018 at 14:36

Any help with this? All noted problems have been fixed.

Comment #34

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 18 August 2018 at 07:13

Priority:

Normal

» Critical

To the reviewers: Please set the priority to Normal after reviewing the project.

Comment #35

sleitner commented 18 August 2018 at 20:01

Priority:	Critical	» Normal
Status:	Needs review	» Reviewed & tested by the community

Automated Review

Review of the 7.x-1.x branch (commit d7eb744):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script.

Manual Review

Individual user account: Yes: Follows the guidelines for individual user accounts.
No duplication: Yes: Does not cause module duplication and/or fragmentation.
Master Branch: Yes: Follows the guidelines for master branch.
Licensing: Yes: Follows the licensing requirements.
3rd party assets/code: Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md: Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review: Yes: Follows the guidelines for project length and complexity.
Secure code: Yes: Meets the security requirements.
Coding style & Drupal API usage: no issues
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Comment #36

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 19 August 2018 at 09:46

Assigned:	Unassigned	» avpaderno
Status:	Reviewed & tested by the community	» Fixed

Thank you for your contribution!

I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.