-
Notifications
You must be signed in to change notification settings - Fork 10
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
[no ticket][risk=no] Supplemented creator information #8969
Changes from 13 commits
afc952d
854e0a3
a440c21
adb2f6d
5021cf4
1c02e1f
086847f
3e19bc0
ea145a8
2d9ac5b
fcef1db
37e4869
03cd8c4
6b9b1a7
ccf9a6f
ca7bf41
ada1d6c
cc4e8b5
3cc4365
0adcebe
d63fc66
6932030
f595bb2
8fa37c9
63ea159
60e4d8f
4f2a936
c91e3ce
becb5dd
f2a0e2d
465553d
097c2fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,8 @@ public interface WorkspaceMapper { | |
@Mapping(target = "displayName", source = "dbWorkspace.name") | ||
@Mapping(target = "terraName", source = "fcWorkspace.name") | ||
@Mapping(target = "googleBucketName", source = "fcWorkspace.bucketName") | ||
@Mapping(target = "creator", source = "dbWorkspace.creator.username") | ||
@Mapping(target = "creator.userName", source = "dbWorkspace.creator.username") | ||
@Mapping(target = "creator.email", ignore = true) // need to work with security before exposing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when we do get this approval, I want to use a new field like contactEmail or institutionalEmail, to be explicit about which email it is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, I added a comment. |
||
@Mapping( | ||
target = "initialCredits.expirationEpochMillis", | ||
source = "dbWorkspace.creator", | ||
|
@@ -124,7 +125,8 @@ default List<WorkspaceResponse> toApiWorkspaceResponseList( | |
ResearchPurpose workspaceToResearchPurpose(DbWorkspace dbWorkspace); | ||
|
||
@Mapping(target = "cdrVersionId", source = "cdrVersion") | ||
@Mapping(target = "creator", source = "creator.username") | ||
@Mapping(target = "creator.userName", source = "creator.username") // need to work with security before exposing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the comment belongs on the next line |
||
@Mapping(target = "creator.email", ignore = true) | ||
@Mapping(target = "initialCredits.expired", source = "dbWorkspace.initialCreditsExpired") | ||
@Mapping( | ||
target = "initialCredits.expirationEpochMillis", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7278,7 +7278,7 @@ components: | |
cdrVersionId: | ||
type: string | ||
creator: | ||
type: string | ||
$ref: '#/components/schemas/User' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately this is not deployment-safe: when the API is deployed with this change, there's a period of time when the UI will be expecting a string here, before the UI is also updated. Adding a field (maybe creatorUser?) and later deleting the original creator after a release would solve this issue. |
||
billingAccountName: | ||
type: string | ||
googleBucketName: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
import org.pmiops.workbench.db.model.DbWorkspace; | ||
import org.pmiops.workbench.model.BillingStatus; | ||
import org.pmiops.workbench.model.ResearchPurpose; | ||
import org.pmiops.workbench.model.User; | ||
import org.pmiops.workbench.model.Workspace; | ||
import org.pmiops.workbench.model.WorkspaceAccessLevel; | ||
import org.pmiops.workbench.utils.mappers.CommonMappers; | ||
|
@@ -113,14 +114,17 @@ public void setUp() { | |
.researchOutcomeList(Collections.emptyList()); | ||
final long now = System.currentTimeMillis(); | ||
|
||
User creator = new User(); | ||
creator.setEmail("[email protected]"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -> userName |
||
|
||
workspace1 = | ||
new Workspace() | ||
.etag("etag_1") | ||
.name("DbWorkspace 1") | ||
.terraName("dbworkspace1") | ||
.namespace("aou-rw-local1-c4be869a") | ||
.cdrVersionId("1") | ||
.creator("[email protected]") | ||
.creator(creator) | ||
.billingAccountName("big-bux") | ||
.googleBucketName("bucket o' science") | ||
.accessTierShortName(AccessTierService.REGISTERED_TIER_SHORT_NAME) | ||
|
@@ -279,6 +283,8 @@ public void testFireEditAction_sendsChangedProperties() { | |
.timeReviewed(workspace1.getResearchPurpose().getTimeReviewed() + 1000L) | ||
.controlSet(!workspace1.getResearchPurpose().isControlSet()); | ||
final int rpChanges = 5; | ||
User creator = new User(); | ||
creator.setUserName("[email protected]"); | ||
|
||
Workspace editedWorkspace = | ||
clone(workspace1) | ||
|
@@ -287,7 +293,7 @@ public void testFireEditAction_sendsChangedProperties() { | |
// changes | ||
.name("a new name") | ||
.namespace("a new namespace") | ||
.creator("[email protected]"); | ||
.creator(creator); | ||
final int wsChanges = 3; | ||
|
||
workspaceAuditor.fireEditAction(workspace1, editedWorkspace, dbWorkspace1.getWorkspaceId()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import org.pmiops.workbench.access.AccessTierService; | ||
import org.pmiops.workbench.model.ResearchPurpose; | ||
import org.pmiops.workbench.model.SpecificPopulationEnum; | ||
import org.pmiops.workbench.model.User; | ||
import org.pmiops.workbench.model.Workspace; | ||
|
||
class TargetPropertyExtractorTest { | ||
|
@@ -16,6 +17,8 @@ class TargetPropertyExtractorTest { | |
@BeforeEach | ||
void setUp() { | ||
long now = System.currentTimeMillis(); | ||
User creator = new User(); | ||
creator.setEmail("[email protected]"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. userName |
||
|
||
var researchPurpose1 = | ||
new ResearchPurpose() | ||
|
@@ -52,7 +55,7 @@ void setUp() { | |
.terraName("dbworkspace1") | ||
.namespace("aou-rw-local1-c4be869a") | ||
.cdrVersionId("1") | ||
.creator("[email protected]") | ||
.creator(creator) | ||
.accessTierShortName(AccessTierService.REGISTERED_TIER_SHORT_NAME) | ||
.researchPurpose(researchPurpose1); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
import org.pmiops.workbench.access.AccessTierService; | ||
import org.pmiops.workbench.model.ResearchPurpose; | ||
import org.pmiops.workbench.model.SpecificPopulationEnum; | ||
import org.pmiops.workbench.model.User; | ||
import org.pmiops.workbench.model.Workspace; | ||
|
||
public class WorkspaceTargetPropertyTest { | ||
|
@@ -42,13 +43,15 @@ public void setUp() { | |
.anticipatedFindings("a 4-leaf clover"); | ||
|
||
long now = System.currentTimeMillis(); | ||
User creator = new User(); | ||
creator.setEmail("[email protected]"); | ||
|
||
workspace1 = | ||
new Workspace() | ||
.name("Workspace 1") | ||
.terraName("workspace1") | ||
.namespace("aou-rw-local1-c4be869a") | ||
.creator("[email protected]") | ||
.creator(creator) | ||
.cdrVersionId("1") | ||
.researchPurpose(researchPurposeAllFieldsPopulated) | ||
.creationTime(now) | ||
|
@@ -61,7 +64,7 @@ public void setUp() { | |
.name("Workspace 2") | ||
.terraName("workspace2") | ||
.namespace("aou-rw-local1-c4be869a") | ||
.creator("[email protected]") | ||
.creator(creator) | ||
.cdrVersionId("33") | ||
.researchPurpose(researchPurpose2) | ||
.creationTime(now) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,7 @@ | |
import org.pmiops.workbench.model.ResearchPurpose; | ||
import org.pmiops.workbench.model.SexAtBirthV2; | ||
import org.pmiops.workbench.model.SexualOrientationV2; | ||
import org.pmiops.workbench.model.User; | ||
import org.pmiops.workbench.model.Workspace; | ||
import org.pmiops.workbench.model.YesNoPreferNot; | ||
import org.pmiops.workbench.rawls.model.RawlsWorkspaceAccessLevel; | ||
|
@@ -119,6 +120,12 @@ public static Workspace createWorkspace( | |
List<ResearchOutcomeEnum> ResearchOutcomeEnumsList = new ArrayList<>(); | ||
ResearchOutcomeEnumsList.add(ResearchOutcomeEnum.IMPROVED_RISK_ASSESMENT); | ||
|
||
User creator = new User(); | ||
creator.setEmail("[email protected]"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rm, to ensure that this field is not actually what we're reading |
||
creator.setUserName("[email protected]"); | ||
creator.setGivenName("Jay"); | ||
creator.setFamilyName("Tester"); | ||
|
||
return new Workspace() | ||
.etag("\"1\"") | ||
.name(workspaceDisplayName) | ||
|
@@ -131,7 +138,7 @@ public static Workspace createWorkspace( | |
.billingAccountName(WORKSPACE_BILLING_ACCOUNT_NAME) | ||
.googleProject(DEFAULT_GOOGLE_PROJECT) | ||
.creationTime(1588097211621L) | ||
.creator("[email protected]") | ||
.creator(creator) | ||
.creationTime(Instant.parse("2000-01-01T00:00:00.00Z").toEpochMilli()) | ||
.lastModifiedTime(1588097211621L) | ||
.googleProject(DEFAULT_GOOGLE_PROJECT) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,8 @@ | |
public class WorkspaceMapperTest { | ||
private static final String FIRECLOUD_NAMESPACE = "aou-xxxxxxx"; | ||
private static final String CREATOR_EMAIL = "[email protected]"; | ||
private static final String CREATOR_GIVEN_NAME = "Oscar"; | ||
private static final String CREATOR_FAMILY_NAME = "Calhoun"; | ||
private static final long CREATOR_USER_ID = 101L; | ||
private static final long WORKSPACE_DB_ID = 222L; | ||
private static final int WORKSPACE_VERSION = 2; | ||
|
@@ -145,6 +147,8 @@ public void setUp() { | |
final DbUser creatorUser = | ||
new DbUser() | ||
.setUsername(CREATOR_EMAIL) | ||
.setGivenName(CREATOR_GIVEN_NAME) | ||
.setFamilyName(CREATOR_FAMILY_NAME) | ||
.setUserId(CREATOR_USER_ID) | ||
.setUserInitialCreditsExpiration( | ||
new DbUserInitialCreditsExpiration() | ||
|
@@ -213,7 +217,9 @@ public void testConvertsDbToApiWorkspace() { | |
assertThat(ws.getName()).isEqualTo(WORKSPACE_AOU_NAME); | ||
assertThat(ws.getNamespace()).isEqualTo(FIRECLOUD_NAMESPACE); | ||
assertThat(ws.getCdrVersionId()).isEqualTo(Long.toString(CDR_VERSION_ID)); | ||
assertThat(ws.getCreator()).isEqualTo(CREATOR_EMAIL); | ||
assertThat(ws.getCreator().getUserName()).isEqualTo(CREATOR_EMAIL); | ||
assertThat(ws.getCreator().getGivenName()).isEqualTo(CREATOR_GIVEN_NAME); | ||
assertThat(ws.getCreator().getFamilyName()).isEqualTo(CREATOR_FAMILY_NAME); | ||
assertThat(ws.getInitialCredits().getExpirationEpochMillis()) | ||
.isEqualTo(INITIAL_CREDITS_EXPIRATION_TIMESTAMP.getTime()); | ||
assertThat(ws.getGoogleBucketName()).isEqualTo(FIRECLOUD_BUCKET_NAME); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,8 +494,8 @@ export const WorkspaceShare = fp.flow(withUserProfile())( | |
> | ||
When you share this workspace as a ‘Writer’ or an ‘Owner’, | ||
the initial credits of the creator of the workspace ( | ||
{this.props.workspace.creator}) will be used for all | ||
analysis in this workspace. | ||
{this.props.workspace.creator.userName}) will be used for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are a few other instances of this which need changing:
|
||
all analysis in this workspace. | ||
</div> | ||
)} | ||
</ModalTitle> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ export class FeaturedWorkspacesApiStub extends FeaturedWorkspaceApi { | |
return { | ||
workspace: { | ||
...workspace, | ||
creator: '[email protected]', | ||
creator: { userName: '[email protected]' }, | ||
featuredCategory: category as FeaturedWorkspaceCategory, | ||
}, | ||
accessLevel: WorkspaceAccessLevel.OWNER, | ||
|
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.
please switch to
getUserName()
because email is deprecated. We want to stop using "email" in most cases because it's ambiguous (contact email or username?) and we sometimes get it wrong.I have a new PR to remove it (and also update UserRole): #8989
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.
Thanks for pointing this out