Skip to content
This repository was archived by the owner on Apr 2, 2024. It is now read-only.

Conversation

@andriyun
Copy link
Contributor

@andriyun andriyun commented Apr 5, 2022

Followup for PR130

  • New token ['webform_submission:'os2forms_forloeb_execute_task]
  • Refactored assigned task fetching method
  • Added ability to open os2forms_forloeb_execute_task URL without being logged in as Drupal User

@andriyun andriyun requested a review from rimi-itk April 5, 2022 06:00
@andriyun andriyun changed the title OS2FORMS-371 Added token for easy getting os2forms execute task link in mail handlers OS2FORMS-371 Added easy access to assigned maestro task by token Apr 27, 2022
@andriyun andriyun requested a review from rimi-itk April 28, 2022 11:01
Copy link
Collaborator

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Looks good. === should be used for comparisons rather than just ==.

function os2forms_forloeb_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account) {
if ($operation == 'update' && $entity instanceof WebformSubmission) {
$token = \Drupal::request()->query->get('os2forms-forloeb-ws-token');
if ($token && $token == $entity->getToken()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, we should always use === for comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// Catch os2forms-forloeb access token and pass it further.
if ($form instanceof RedirectResponse && $token = \Drupal::request()->query->get('os2forms-forloeb-ws-token')) {
// Check token to previous submission and update it to new one.
if ($token == $webform_submission->getToken()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use === here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@andriyun, do you have local commits that haven't beed pushed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rimi-itk I just pushed fixes for these two changes you requested

@andriyun andriyun requested a review from rimi-itk May 17, 2022 12:52
Copy link
Collaborator

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Very good job.

@andriyun
Copy link
Contributor Author

@rimi-itk I can not go further with it.
I have no access to merge this PR.

@rimi-itk
Copy link
Collaborator

Hmm … I cannot merge either, @andriyun. @mBoegvald & @madsnorgaard, can you help us out?

@madsnorgaard
Copy link
Collaborator

I will squash and merge. Hope you are all well!

@madsnorgaard madsnorgaard merged commit cfb830a into develop May 17, 2022
@skifter skifter deleted the OS2FORMS-371 branch January 31, 2023 13:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants