Skip to content

ISSUE-217: Ensures info['uid'] to be present for both processing options#218

Merged
DiegoPino merged 2 commits into
0.8.0from
ISSUE-217
Sep 5, 2024
Merged

ISSUE-217: Ensures info['uid'] to be present for both processing options#218
DiegoPino merged 2 commits into
0.8.0from
ISSUE-217

Conversation

@DiegoPino
Copy link
Copy Markdown
Member

See #217

@DiegoPino DiegoPino added the bug Something isn't working label Sep 5, 2024
@DiegoPino DiegoPino added this to the 0.8.0 milestone Sep 5, 2024
@DiegoPino DiegoPino self-assigned this Sep 5, 2024
@DiegoPino
Copy link
Copy Markdown
Member Author

@patdunlavey can you please give this a check?

DiegoPino referenced this pull request Sep 5, 2024
…perations in AmiUtilityService::preprocessAmiSet and AmiUtilityService::getProcessedAmiSetNodeUUids. Fixes cases where the process is executing under cron or hydroponics and the user is anonymous. (#215)
}
if ($file && $data!== new \stdClass()) {
// Only UUIDs you can delete will be added.
$data->info['uid'] = $this->currentUser()->id();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The ami set has a uid value - the author of the set. I'm not sure which uid makes more sense: the author of the ami set, or the user currently processing the set?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The user processing in my opinion ... At least that has been the behavior. If we want to change that we can introduce a new feature/option in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That works for me - just wanted to make sure it was considered!

I have an AMI set processing (successfully - no white screen) on my local now. After that, I'll test deleting processed ADOs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@DiegoPino CONFIRMED: both processing and deleting processed ADOs are successful!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also checked that enqueued csv expansion and ado processing still work, and they do.

Copy link
Copy Markdown
Contributor

@patdunlavey patdunlavey left a comment

Choose a reason for hiding this comment

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

Confirmed in local testing

@patdunlavey
Copy link
Copy Markdown
Contributor

Do you need a PR to get this merged to 0.9.0?

DiegoPino added a commit that referenced this pull request Sep 5, 2024
…ons (#218)

* Ensures info['uid'] to be present for both CSV expander and direct processing

* Also for delete operation
DiegoPino added a commit that referenced this pull request Sep 12, 2024
…ons (#218) (#219)

* Ensures info['uid'] to be present for both CSV expander and direct processing

* Also for delete operation
@DiegoPino DiegoPino deleted the ISSUE-217 branch October 11, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants