Skip to content

MS-1058 Prep work for sample upload with signed urls#1251

Merged
luhmirin-s merged 7 commits into
mainfrom
feature/MS-1058-sample-upload
Jul 10, 2025
Merged

MS-1058 Prep work for sample upload with signed urls#1251
luhmirin-s merged 7 commits into
mainfrom
feature/MS-1058-sample-upload

Conversation

@luhmirin-s
Copy link
Copy Markdown
Contributor

@luhmirin-s luhmirin-s commented Jul 7, 2025

JIRA ticket
Will be released in: 2025.3.0

Notable changes

  • This PR adds some secondary changes for future refactoring of the sample upload. The actual upload logic requires much more changes than was anticipated and therefore will be done in a separate PR.
    • Added configuration fields for future use in sample upload.
    • Added a new event and scope type for sample upload.
    • Added additional fields to upsync event.

Testing guidance

  • Poke around and check that nothing is broken. And that configuration has migrated correctly after update/install.

Additional work checklist

  • Effect on other features and security has been considered
  • Design document marked as "In development" (if applicable)
  • External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
  • Test cases in Testiny are up to date (or ticket created)
  • Other teams notified about the changes (if applicable)

@cla-bot cla-bot Bot added the ... label Jul 7, 2025
@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch 2 times, most recently from 7e70ab2 to 822814a Compare July 8, 2025 07:51
@luhmirin-s luhmirin-s changed the title MS-1058 Sample upload with signed urls MS-1058 Prep work for sample upload with signed urls Jul 8, 2025
@luhmirin-s luhmirin-s requested review from a team, BurningAXE, TristramN, alex-vt, alexandr-simprints, meladRaouf and ybourgery and removed request for a team July 8, 2025 08:21
@luhmirin-s luhmirin-s marked this pull request as ready for review July 8, 2025 08:21
@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch from 822814a to 9c589d3 Compare July 8, 2025 12:23
@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch from 9c589d3 to 1d6f00a Compare July 8, 2025 12:39
@luhmirin-s luhmirin-s requested a review from ybourgery July 9, 2025 08:12
@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch from 6b30ab0 to 4f924ac Compare July 9, 2025 09:36
@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch from 4f924ac to 551f5aa Compare July 9, 2025 10:11
!synchronization.hasSamples() ||
synchronization.up.simprints.batchSizes
.let { it.eventUpSyncs == 0 || it.eventDownSyncs == 0 }
}
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.

This is just a general comment

Don't you find such conditions difficult to read? While it is a relatively short notation, wouldn't it be better to write such conditions in more human-friendly format?

with (currentData) {
    val hasNoSamples = !synchronization.hasSamples()
    val batchSizeIsEmpty = synchronization.up.simprints.batchSizes
                  .let { it.eventUpSyncs == 0 || it.eventDownSyncs == 0 }
    return@with hasNoSamples || batchSizeIsEmpty
}

maxAge = DownSynchronizationConfiguration.DEFAULT_DOWN_SYNC_MAX_AGE,
),
samples = SampleSynchronizationConfiguration(
signedUrlBatchSize = 5,
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.

Should move it to a constant field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, although this is a very old migration, and there is little difference.

content: EventUpSyncRequestEvent.UpSyncContent,
) {
if (content.sessionCount > 0 || content.eventDownSyncCount > 0 || content.eventUpSyncCount > 0) {
if (content.sessionCount > 0 || content.eventDownSyncCount > 0 || content.eventUpSyncCount > 0 || content.sampleUpSyncCount > 0) {
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.

Maybe it's time to refactor it a bit

val shouldAdd = listOf(content.sessionCount, content.eventDownSyncCount, content.eventUpSyncCount, content.sampleUpSyncCount).any { it > 0 }
if (shouldAdd) { ... }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will move the check to a method in the content class to shorten the access.

In general, for string/null check I would agree. Making a full list object to iterate over a couple of boolean flags just feels wrong, tho. :D

@luhmirin-s luhmirin-s force-pushed the feature/MS-1058-sample-upload branch from 299fa10 to 83a7b62 Compare July 10, 2025 14:09
@sonarqubecloud
Copy link
Copy Markdown

@luhmirin-s luhmirin-s merged commit 1330f72 into main Jul 10, 2025
12 checks passed
@luhmirin-s luhmirin-s deleted the feature/MS-1058-sample-upload branch July 10, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants