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

Remove user id from filepath #1161

Merged

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Dec 3, 2024

Related issue(s) and PR(s)
This PR closes #1097 .

Description
The user's id is no longer stored in the database, so that it will never be shown to a user of download.

  • filepaths in integration tests are updated to reflect this change
  • inbox does not store the user id
  • ingest and mapper do not expect the user id to be there

How to test

Note
The db schema version from #1123 should be set, but potentially in another PR.
The base of this PR is feature/do-not-leak-user-ids, which is branched off main. I do not feel strongly about the naming of this branch, so feel totally free to have opinions.

@MalinAhlberg MalinAhlberg force-pushed the feature/remove-user-from-filepath branch 2 times, most recently from 69a9ac5 to c1c8db9 Compare December 5, 2024 07:39
@MalinAhlberg MalinAhlberg force-pushed the feature/remove-user-from-filepath branch from c1c8db9 to 1acdb43 Compare December 6, 2024 12:25
@MalinAhlberg MalinAhlberg marked this pull request as ready for review December 6, 2024 12:27
@MalinAhlberg MalinAhlberg requested a review from a team December 6, 2024 12:27
@MalinAhlberg MalinAhlberg marked this pull request as draft December 6, 2024 12:46
@MalinAhlberg MalinAhlberg changed the base branch from main to feature/do-not-leak-user-ids December 12, 2024 13:37
@MalinAhlberg MalinAhlberg self-assigned this Dec 12, 2024
@MalinAhlberg MalinAhlberg force-pushed the feature/remove-user-from-filepath branch from 3e4add6 to 6e3e439 Compare December 13, 2024 09:59
@MalinAhlberg MalinAhlberg marked this pull request as ready for review December 13, 2024 10:29
@MalinAhlberg
Copy link
Contributor Author

Thanks for the quick review, @jbygdell ! And sorry for making additional commits... but I felt I needed to add 7dc878a to improve the log messages from the integrations tests. It's a very small update.

Copy link
Contributor

@aaperis aaperis 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! Some minor comments/questions :-)

.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved
sda/cmd/mapper/mapper.go Show resolved Hide resolved
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

Nice work!

@MalinAhlberg MalinAhlberg merged commit 0b2d6c7 into feature/do-not-leak-user-ids Dec 17, 2024
25 checks passed
@MalinAhlberg MalinAhlberg deleted the feature/remove-user-from-filepath branch December 17, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify filepath not storing the user id part (to be continued)
3 participants