Hi everyone :)

I created this module to be used with an elearning distribution we created called Opigno. https://drupal.org/project/opigno_lms
The distribution installs by default some modules we created, while some others are optional using the Apps module. This particular module is an optional one. All the modules can be found here. https://drupal.org/search/site/Opigno?f[0]=ss_meta_type%3Amodule
The purpose of this module is to integrate the video module with our distribution, but it can also be installed without the distribution. It creates a content type called Video, a view and sets default permissions if in the Opigno distribution context.
Og module is required and the content type is set as an Organic Group content.

Project link: https://drupal.org/sandbox/Jamesap/2112951
Git repository: http://git.drupal.org/sandbox/Jamesap/2112951.git

Best regards
James Aparicio

Comments

Anonymous’s picture

Anonymous (not verified) commented

Hi there,

A few comments:

There is a still master branch you should delete that master branch (if you can)

https://drupal.org/empty-git-master

see the git branch -D master

then add a readme file

and then some codesniffer erros

as follows:

http://pareview.sh/pareview/httpgitdrupalorgsandboxjamesap2112951git

Best of luck,
/K

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work
Jamesap’s picture

Jamesap commented
Status: Needs work » Needs review

Hi kentoro, thanks for the review, i done the changes you recommended, i used the coder module before posting here, but it didnt signal those best practices. Nice to know of this tool pareview.sh.
The only one that is still remaining from the reviewer is
94 | WARNING | Unused variable $translatables.
It is regarding the view, the way it does the export. Isnt this the correct way to do it?

Best regards

Anonymous’s picture

Anonymous (not verified) commented

2 things here.
1.- Good job that is just a warning, that as the comment suggests might or may not be a false positive.
2.- It's complaining that the variable is never being used not the definition per se.

Now I'd like to see an example in another module about the translatables being used as you did.
The only reference i could find is :
https://api.drupal.org/api/views/includes!base.inc/function/views_object...

Anonymous’s picture

Anonymous (not verified) commented

In your .install file you need to add

the uninstall function

and delete the variables your module is using

with variable_del() that is.

Greetings.
/K

Anonymous’s picture

Anonymous (not verified) commented
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

I'm putting needs work untill the uninstall function is done.

Jamesap’s picture

Jamesap commented
Status: Needs work » Needs review

Hi kentoro, thanks for the feedback :)
Regarding the $translatables array, from my understanding, views exports that array so when importing the view, it lets Drupal know that there are those translatable strings, even if no one requests the view in question. If i remove the array i will have to wait for someone to request the view for me to be able to translate those strings. Do you think i should remove the array? It really doesnt make that much of a difference to me.

Best regards

Jamesap’s picture

Jamesap commented

Oh ok, ill work on the uninstall function :)

Jamesap’s picture

Jamesap commented
Status: Needs review » Needs work
Anonymous’s picture

Anonymous (not verified) commented

Since it's a warning and not module breaker i won't mind, but i don't know if the finals reviewer/or someone else wil, You'll have to wait and see :) - good job so far!

Jamesap’s picture

Jamesap commented
Status: Needs work » Needs review

Added hook_uninstall.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community
Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes

Fixed git link

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Project page
Please take a moment to make your project page follow tips for a great project page.
README.txt
Please take a moment to make your README.txt follow the guidelines for in-project documentation.

No major problems found, thanks for your contribution, Jamesap!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.