Skip to content
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

Copy changes for general settings #36

Closed
wants to merge 7 commits into from
Closed

Copy changes for general settings #36

wants to merge 7 commits into from

Conversation

cnovak
Copy link
Collaborator

@cnovak cnovak commented Jun 30, 2018

  • Copy changes to general settings
  • Change default name of “developer apps” to “apps”
  • “Apigee Edge” should not be shortened to “Edge”
  • Remove custom styling of developer sync page, change to match cron form
  • Removed “please” from copy
  • Page titles should only have first work capitalized

* Copy changes to general settings
* Change default name of “developer apps” to “apps”
* “Apigee Edge” should not be shortened to “Edge”
* Remove custom styling of developer sync page, change to match cron form
* Removed “please” from copy
* Page titles should only have first work capitalized
@cnovak cnovak requested review from earth2marsh, tamasd and mxr576 June 30, 2018 13:38
@apigee apigee deleted a comment from googlebot Jul 2, 2018
@@ -46,8 +46,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
'#title' => $this->t('Sync developers'),
'#open' => TRUE,
];

$form['sync']['description'] = [
'#markup' => '<p>' . $this->t('Developer synchronization will run through all users in this portal, adding them as developers in the Apigee Edge org, and making sure all developers on the Apigee Edge org are added to this portal. The "Run Developer Sync" button will sync the developers, displaying a progress bar. The "Background Developer Sync" button will run the developer sync process in batches each time <a href=":cron_url">cron</a> runs.', [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the #html_tag render element instead if this really needs to wrapped in <p> tag. Is it?

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21Element%21HtmlTag.php/class/HtmlTag/8.5.x

@mxr576 mxr576 mentioned this pull request Jul 2, 2018
@cnovak
Copy link
Collaborator Author

cnovak commented Jul 2, 2018

@mxr576 I followed the same pattern for description that the Cron form uses:

 /**
   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state) {
    $form['description'] = [
      '#markup' => '<p>' . t('Cron takes care of running periodic tasks like checking for updates and indexing content for search.') . '</p>',
    ];
    $form['run'] = [
      '#type' => 'submit',
      '#value' => t('Run cron'),
      '#submit' => ['::runCron'],
    ];

The code and other forms are following this format, so if we don't put description in

the page may style differently than other pages. Let me know if you still think we should change this code to use #html_tag render element. I think we should stay with the pattern other admin forms are using.

@cnovak
Copy link
Collaborator Author

cnovak commented Jul 2, 2018

Talked to @mxr576 , will still wrap in

like other modules, but use #html_tag render element for more flexibility.

@mxr576 mxr576 requested a review from peterserfozo July 3, 2018 15:27
$form['sync']['description'] = [
'#type' => 'html_tag',
'#tag' => 'p',
'#value' => $this
Copy link
Contributor

@mxr576 mxr576 Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little nitpicking, but single method call should be in the same line, parameter list could be in multiple lines if it is necessary, otherwise, PR looks fine.

@mxr576
Copy link
Contributor

mxr576 commented Jul 4, 2018

@cnovak Please monitor your Travis builds after you created a new PR every time. It was quite clear that your code changes introduced a new a coding style violation: https://travis-ci.org/cnovak/apigee-edge-drupal/builds/399633201

As a general practice I'd like to ask all developers to submit their Travis build in the URL of their PR so we could easily check whether their build is passing or not. The apigee/apigee-edge-drupal Travis CI is only checking code style, but it does not run tests with active Apigee Edge connection.

@apigee apigee deleted a comment from googlebot Jul 4, 2018
@mxr576
Copy link
Contributor

mxr576 commented Jul 4, 2018

@cnovak Some tests are starting to get failed on your Travis after I have fixed the code style: https://travis-ci.org/cnovak/apigee-edge-drupal/jobs/399868817#L1173

Copy link
Contributor

@mxr576 mxr576 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing tests on Travis should be fixed.

@cnovak
Copy link
Collaborator Author

cnovak commented Jul 6, 2018

I ran the build again after changing connection settings to fix the failed build, and still getting errors:

Caused by
Apigee\Edge\Exception\ClientErrorException: Forbidden

Not much info to go on. What needs to be done to troubleshoot this error?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@mxr576
Copy link
Contributor

mxr576 commented Oct 8, 2018

@cnovak would you like to update this PR or can we close this?

@cnovak
Copy link
Collaborator Author

cnovak commented Oct 10, 2018

I need to finish this in this sprint. Still having an issue where password is getting modified to something different using environment variables. I debugged locally and could see it was being changed but did not have a chance to figure out why.

@peterserfozo
Copy link
Contributor

@cnovak It seems your credentials are wrong in Travis. Try to delete APIGEE_EDGE_USERNAME and APIGEE_EDGE_PASSWORD (https://travis-ci.org/cnovak/apigee-edge-drupal/settings) and set the values again for these environment variables surrounded by double quotes (") then restart the latest build.

@cnovak cnovak closed this Oct 22, 2018
@cnovak cnovak deleted the copychanges-general-1 branch October 22, 2018 23:23
@cnovak
Copy link
Collaborator Author

cnovak commented Oct 22, 2018

Going to resubmit this PR after merging

@cnovak cnovak restored the copychanges-general-1 branch October 25, 2018 00:26
@cnovak cnovak deleted the copychanges-general-1 branch November 9, 2018 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants