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

Algolia integration and Indexing the entities #244

Closed

Conversation

@sandipbhuyan
Copy link
Collaborator

@sandipbhuyan sandipbhuyan commented Jun 22, 2018

Resolves #243. This PR is pointing to install Agolia and indexing the entities Project and ProjectClassification

@sandipbhuyan sandipbhuyan self-assigned this Jun 22, 2018
@sandipbhuyan sandipbhuyan requested review from imphil and recrsn Jun 22, 2018
@sandipbhuyan sandipbhuyan force-pushed the algolia-integration branch 2 times, most recently from f8ee292 to bb047a5 Jun 25, 2018
Copy link
Contributor

@imphil imphil left a comment

I have mentioned some general things I saw when looking through the PR. I'm not yet familiar enough to judge if the Algolia integration actually works or should be done this way. @agathver do you have more experience in this domain?

Loading

@@ -45,3 +45,6 @@ parameters:
# Register at https://sentry.io
sentry_dsn: '{{ site_sentry_dsn }}'
sentry_env: '{{ site_sentry_env }}'

env(ALGOLIA_APP_ID): '{{ site_algolia_app_id }}'
env(ALGOLIA_API_KEY): '{{ site_algolia_api_key }}'
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

Please add a comment what these parameters are for and where to get them from, and end the file with the newline. (like all files, i.e. fix this across this PR).

Loading


# Algolia
site_algolia_app_id: "INSERT_ALGOLIA_APP_ID_HERE"
site_algolia_api_key: "INSERT_ALGOLIA_API_KEY_HERE"
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

Even though it does not change functionality, stick with single quotes (') for consistency. (Multiple times in this PR.)

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jun 26, 2018

Choose a reason for hiding this comment

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

There were some (") quotes inside the same file should I change those too ??

Loading

@@ -6,6 +6,7 @@


# Require confirmation of the email address of newly registered users.
# Require site_algolia_app_id and site_algolia_api_key of algolia application.
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

This looks strange within this mailer comment?

Loading

# Requires a working mailer setup. Add SMTP credentials in
# secrets/dev-vagrant.secrets.yml before enabling this setting.
#site_user_email_confirmation_enabled: true
# site_user_email_confirmation_enabled: true
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

This comment is using intentionally no space after the "#" sign to show that this is not a comment, but a commented-out config setting. This is commonly done in example or preconfigured config files.

Loading

Clear indices
-------------
.. code-block:: bash
qqq
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

remove.

Loading

@@ -110,3 +110,22 @@ Check the coding style of PHP code
vm$> cd /var/www/lc/site
vm$> ./vendor/bin/phpcs --runtime-set ignore_warnings_on_exit true -s \
&& echo You can commit: No errors found!
Algolia indices configuration
---------------------------
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

Please use "-" until the end of the headline.

Loading

Algolia indices configuration
---------------------------
For configuring algolia in development environment first specify the Apllication ID and Admin API Key in ``parameter.yml``
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

"To configure Algolia in the development environment you need to specify the Application ID (site_algolia_app_id) and the Admin API Key (site_algolia_api_key) in the ansible/secrets/dev-vagrant.secrets.yml file."

Is it really called "Admin API Key"?

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jun 26, 2018

Choose a reason for hiding this comment

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

Yes algolia use two keys one for inserting data into algolia(Admin API Key) another for searching functionality (Search-Only API Key)

Loading

Algolia indices configuration
---------------------------
For configuring algolia in development environment first specify the Apllication ID and Admin API Key in ``parameter.yml``
Clear the indices and import the settings from algolia
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

You say here it imports the settings, below you say it imports the indices. What is correct?

Loading

class: Librecores\ProjectRepoBundle\Entity\Project

- name: classifications
class: Librecores\ProjectRepoBundle\Entity\ProjectClassification
Copy link
Contributor

@imphil imphil Jun 25, 2018

Choose a reason for hiding this comment

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

Is it really necessary to specify the classifications as separate index, doesn't this go into the projects index?

Loading

More attributes are being added for normalizing the entity. The PHPDoc comments are being
improved.
Classification  are embeded in Project entity and modification of documentation files
@sandipbhuyan sandipbhuyan force-pushed the algolia-integration branch from 6b8ae90 to dee1383 Jun 28, 2018
@imphil
Copy link
Contributor

@imphil imphil commented Jul 4, 2018

I was going ahead and merge that but now that @agathver has brought up some issues in #246 which we might want to get resolved first in this PR (since the other once just folds in the changes of this one, and needs to be rebased on top of this one after it goes in).

  • I agree with Amitosh about the missing newline at the end of the file. Look through the GitHub diff and the markers at the end of the files when a newline is missing, and add those.
  • I also agree about the @inheritdoc usage.
  • I'm not sure about what is meant by the comment that env(ALGOLIA_APP_ID): and friends are not required. Isn't that the (recommended/documented) way to configure Algolia with this version of Symfony? Somewhere this information is needed, isn't it?

Loading

Copy link
Collaborator

@recrsn recrsn left a comment

Please keep the EOL and new-line at end of the file consistent.

I also covered some additional points in #246

Loading

@@ -45,3 +45,7 @@ parameters:
# Register at https://sentry.io
sentry_dsn: '{{ site_sentry_dsn }}'
sentry_env: '{{ site_sentry_env }}'

# Algolia Configuration
env(ALGOLIA_APP_ID): '{{ site_algolia_app_id }}'
Copy link
Collaborator

@recrsn recrsn Jul 4, 2018

Choose a reason for hiding this comment

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

This part should stay, disregard my comment in #246
Algolia client v3 has changed the configuration interface.

Loading

/**
* Normalizes the object into an array of scalars|arrays.
*
* @param NormalizerInterface $serializer The serializer is given so that you
Copy link
Collaborator

@recrsn recrsn Jul 4, 2018

Choose a reason for hiding this comment

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

Use {@inheritdoc}

Loading

public function normalize(NormalizerInterface $serializer, $format = null, array $context = array())
{
return [
'name' => $this->getName(),
Copy link
Collaborator

@recrsn recrsn Jul 4, 2018

Choose a reason for hiding this comment

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

Will it be possible to show the project code quality score through this API? You may need to create a custom normalizer as described in https://github.com/algolia/search-bundle#using-a-custom-normalizer

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 4, 2018

Choose a reason for hiding this comment

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

I am converting it to custom normalizer

Loading

@sandipbhuyan
Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan commented Jul 5, 2018

@imphil and @agathver I have done some changes in this PR. That might resolve the issues here

  • The custom normalizer is added.
  • use of {@inheritdoc}
  • And the configuration for aloglia-search-bundle require env(ALGOLIA_APP_ID) and env(ALGOLIA_API_KEY) in parameter.yml, So it is not wise to remove these two fields from our application

Loading

@imphil
Copy link
Contributor

@imphil imphil commented Jul 9, 2018

Now the only thing missing is the newline at the end of the file, please remove all occurrences in the diff that show
image
in the diff view in GitHub.

Loading

@sandipbhuyan
Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan commented Jul 9, 2018

@imphil I have missed the new line at the end of the files now that thing is resolved.

Loading

organization_normalizer:
class: Librecores\ProjectRepoBundle\Serializer\Normalizer\OrganizationNormalizer
tag: serializer.normalizer
public: false
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

Are these service registrations really needed? We have enabled all the new auto* registration/wiring functionality of Symfony in preparation for Symfony 4. (Symfony should get the tag from the inheritance of the class, but please double-check.)

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

I have added this field as algolia search bundle configuration have described. I am checking it again for its exsistance

Loading

Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

It's not wrong, but the old way of doing things, before autoconf and autowire existed. We have moved almost the complete codebase to these new features, and should do the same for these three new services. As I said, just remove the service registrations and things should work fine.

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

Thanks, The application is working after removing those configurations

Loading

public: false

framework:
serializer: { enabled: true }
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

Shouldn't that go into the main configuration file instead of the service configuration file?

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

We dont have a serielizer or framework-bundle in our application thats why it requiers this feature in service configuration file. https://github.com/algolia/search-bundle#no-serializer-service-found

Loading

Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

The services.yml file specifies the services that are available. Other configuration goes typically into config.yml. This is also how the Symfony docs do it: https://symfony.com/doc/3.4/serializer.html

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

If I am adding these feature in the main configuation exception is being shown by symfony
screen shot 2018-07-09 at 8 00 21 pm

Loading

Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

services.yml is included in config.yml (

- { resource: services.yml }
). So the result should be exactly the same -- if not then something else is wrong. Make sure to add it in the existing 'framework' key in config.yml.

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

Thanks. Resolved.

Loading

*
* {@inheritdoc}. This class normalizes the Projects for getting indexed in algolia
*
* @author Sandip Kumar Bhuyan <sandipbhuyan@gmail.com>
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

We don't add an author to a function doc comment. You can add it to the class doc comment or remove it.

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

rectifying it, can I add these at the class name?

Loading

Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

That's what I wrote, yes.

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

done

Loading

*
* {@inheritdoc}. This class normalize the users for getting indexed in algolia.
*
* @author Sandip Kumar Bhuyan <sandipbhuyan@gmail.com>
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

same as above.

Loading

-----------------------------
To configure Algolia in the development environment you need to specify the Application ID (site_algolia_app_id)
and the Admin API Key (site_algolia_api_key) in the ansible/secrets/dev-vagrant.secrets.yml file.
Clear and import the search settings from algolia
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

Is this the start of a sentence, or is something else missing?

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

replacing it to you have to clear and import the search indices settings for pushing data to algolia.

Loading


# Algolia Configuration
env(ALGOLIA_APP_ID): '{{ site_algolia_app_id }}'
env(ALGOLIA_API_KEY): '{{ site_algolia_api_key }}'
Copy link
Contributor

@imphil imphil Jul 9, 2018

Choose a reason for hiding this comment

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

add a newline here please

Loading

Copy link
Collaborator Author

@sandipbhuyan sandipbhuyan Jul 9, 2018

Choose a reason for hiding this comment

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

I am adding a new line in parameters.yml.dist file too.

Loading

…nts in Normalizers and rectify the tipsandtricks documentations
@imphil
Copy link
Contributor

@imphil imphil commented Jul 9, 2018

squashed and merged in c784066. Thanks for your work on this Sandip!

Loading

@imphil imphil closed this Jul 9, 2018
Improve the LibreCores.org in terms of discoverability automation moved this from Review to Done Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants