This project is a translation module for TMGMT that will connect our customers using Drupal to our translations management system and send us all required files for translation.

Project link

https://www.drupal.org/project/tmgmt_morningside

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/tmgmt_morningside.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_mornings...

CommentFileSizeAuthor
#13 tmgmt_morningside_coding.png114.39 KBgurubaskar
#8 Morningside_Translator-3106120-#5.patch3.23 KBgurubaskar

Comments

gurubaskar created an issue. See original summary.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
commented
Issue summary: View changes

Hi @gurubaskar,

Thank you for contribution!!!

This below link will help you in resolving all the issue and also I am adding it in the summary. First resolve all, then again reopen to 'Needs Review'.
https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_mornings...

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
commented
Status: Needs review » Needs work
gurubaskar’s picture

gurubaskar
Location Bangalore
commented
Status: Needs work » Needs review

Hi @rksyravi

Thanks for your review.

1. I have fixed README file, hook_help() and Coding standards.
2. We are not using PHPUnit test case. So, we ignore it.
3. I have tried to execute my git in parview.sh, but its keep on executing.

Please review it.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented

Still some issue on pareview.sh, check at https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_mornings...

DrupalPractice has found some issues with your code but could be false positives.

FILE: .../drupal/pareviewsh/pareview_temp/src/UpdateRequestStatusServices.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
26 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
147 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
--------------------------------------------------------------------------

FILE: .../pareview_temp/src/Plugin/tmgmt/Translator/MorningsideTranslator.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
244 | WARNING | Variable $path is undefined.
284 | WARNING | Unused variable $send.
--------------------------------------------------------------------------

Time: 1.68 secs; Memory: 10Mb

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Status: Needs review » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
gurubaskar’s picture

gurubaskar
Location Bangalore
commented
Status: Needs work » Needs review
StatusFileSize
new Morningside_Translator-3106120-#5.patch3.23 KB

Hi @ankushgautam76@gmail.com,

Thanks for your review.

The issue raised by you are already fixed. When I execute the parview.sh, I am getting below errors.

FILE: ...000000/site1101/web/vendor/drupal/pareviewsh/pareview_temp/README.md
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
62 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...endor/drupal/pareviewsh/pareview_temp/tmgmt_morningside.services.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
4 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: .../drupal/pareviewsh/pareview_temp/src/UpdateRequestStatusServices.php
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
35 | ERROR | Parameter $database is not described in comment
--------------------------------------------------------------------------

FILE: .../web/vendor/drupal/pareviewsh/pareview_temp/tmgmt_morningside.module
--------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------
9 | WARNING | [x] Unused use statement
23 | ERROR | [x] Expected 1 space between comma and "':sources'"; 0
| | found
23 | ERROR | [x] Expected one space after the comma, 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Please apply the patch and update your review.

All the functions are using APIs and creating actual entry to Morning side in the third party. Hence could not provide test case. PHPUnit test.

rksyravi’s picture

rksyravi
Primary language Hindi
Location New Delhi, 🇮🇳
commented

Hi @gurubaskar,

Here you should not provide any patch.
If you have already fixed the above-mentioned issue then push that code to your project branch and then change the status to Needs Review, so that all the active members can review and give you the feedback.

gurubaskar’s picture

gurubaskar
Location Bangalore
commented

Hi @rksyravi,

I have commit and pushed all the code to dev branch. Please review it.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented

@gurubaskar

Minor Pareview issue pending :

FILE: ...000000/site1101/web/vendor/drupal/pareviewsh/pareview_temp/README.md
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
62 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...endor/drupal/pareviewsh/pareview_temp/tmgmt_morningside.services.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
4 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: .../drupal/pareviewsh/pareview_temp/src/UpdateRequestStatusServices.php
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
35 | ERROR | Parameter $database is not described in comment
--------------------------------------------------------------------------

FILE: .../web/vendor/drupal/pareviewsh/pareview_temp/tmgmt_morningside.module
--------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------
9 | WARNING | [x] Unused use statement
23 | ERROR | [x] Expected 1 space between comma and "':sources'"; 0
| | found
23 | ERROR | [x] Expected one space after the comma, 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 4.47 secs; Memory: 12Mb

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Status: Needs review » Needs work
gurubaskar’s picture

gurubaskar
Location Bangalore
commented
Status: Needs work » Needs review
StatusFileSize
new tmgmt_morningside_coding.png114.39 KB

Hi @ankushgautam76@gmail.com,

Can you please let me know how you are able to get these issues so that I can also reproduce it and fix it. I am unable to get these in this review

Please check below link and attached screenshot for your reference.

https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_mornings...

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

At the moment, pareview.sh isn't working. When I try it, it throws an exception, or it doesn't run the tests and it shows (I think) a default message.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

On a quick preview, it seems that what reported from the previous comment was already fixed. I cannot speak about the empty line at the end of the files, since the Gitlab viewer doesn't seem to show them.

imclean’s picture

imclean commented

pareview.sh is now up and running. I requested a "Repeat review" and there are no errors or warnings.

imclean’s picture

imclean commented

https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_mornings...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Thanks for you contribution!

  1. tmgmt_morningside_install: all state variables that you set need to be prefixed with your module name to avoid name clashes with other modules. Please check how other Drupal core code uses the state system.
  2. tmgmt_morningside_schema(): primary keys for all tables are missing. Is that intentional?
  3. "\Drupal::logger('tmgmt_morningside')->error('Morningside translator service returned the following error while creating request: ' . $error['Message'], []);": do not concatenate variables to log strings, use placeholder instead. See https://drupalize.me/blog/201510/how-log-messages-drupal-8
  4. The AES composer library for encryption looks a bit fishy, but since this is only used for sending requests to the translation service I think we don't have to care.

Did not see any security issues, looks good to me.

gurubaskar’s picture

gurubaskar
Location Bangalore
commented

Hi @klausi,

Thanks for reviewing.

I have fixed the points mentioned at #19 by you. In Point 2 one database table require primary key that has updated. Rest not required in our scenario.

Great, thank you everyone for reviewing. What's the next step?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

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.

gurubaskar’s picture

gurubaskar
Location Bangalore
commented

Thank you, everyone for your comments and suggestions.

Status: Fixed » Closed (fixed)

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