-
Notifications
You must be signed in to change notification settings - Fork 9
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
Move user fields into a sub-module #89
Conversation
07afc49
to
5a94bba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this according to the instructions. Steps I took:
- I built a blank website according to the instructions provided in the oe_authentication module.
- I used the docker-compose.yml docker containers in order to run the authentication mock.
- In the master branch, I logged in as
Chuck NORRIS
, one of the mock users. - I then logged in with an admin account and verified that there were no errors in the status report, the user
Chuck NORRIS
was blocked, and the base property fields retrieved and stored the values in the user account. - Checked out the current branch and executed the updated.
- Performed the same checks to verify that everything is ok.
- Rebuilt the cache just to be sure and rechecked everything.
All tests went just like it was explained it would. I only have some commenting nitpicks.
/** | ||
* Implements hook_entity_base_field_info(). | ||
* | ||
* Add custom EU Login fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds custom...
*
* Add custom EU Login fields. | ||
*/ | ||
function oe_authentication_user_fields_entity_base_field_info(EntityTypeInterface $entity_type): array { | ||
if ($entity_type->id() != 'user') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use !==
here.
]; | ||
|
||
/** | ||
* Converts an array EU Login attributes into an array of Drupal field/values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converts an array of* EU Login....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this a bit since it was exceeding the 80 lines limit.
* An array containing a series of EU Login attributes. | ||
* | ||
* @return array | ||
* An array containing a series of Drupal field names and values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it more usual in Drupal to describe this as something like "An associative array of field values indexed by the field name"?
* The triggered event. | ||
* | ||
* @throws \Drupal\Core\Entity\EntityStorageException | ||
* In case of failures an exception is thrown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception class seems too specific for a broad exception message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am removing this entirely referring back to a discussion between Antonio and Pieter about not trying to document or catch exceptions related to the entity storage operations unless we are passing them as parameters when retrieving them.
I discussed with @claudiu-cristea and since he is already occupied with another task and these are simply nitpicks, I would implement them after him agreeing with them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase.
c2e78e4
to
6827a8a
Compare
Fixes #81
QA notes
Clean install
Is covered by the Behat tests.
Testing the update path
Install de development environment according to the README.md.
Go to
/admin/reports/status
and check that there are no Entity/Field Definitions mismatch errors.Do a login for a test user (see the eCAS Mock Server users data) and, in the
users_field_data
table, check that the profile fields (field_oe_*
) are correctly filled.Refresh
/admin/reports/status
and check that there are no Entity/Field Definitions mismatch errors.Check the
users_field_data
table. The fields should be filled with the previous data.