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

Removed outdated logic for determining survey creation status #1969

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions proto/src/ground/v1beta1/survey.proto
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,21 @@ message Survey {
// Defines the terms data collectors must agree to before
// collecting data.
DataSharingTerms data_sharing_terms = 6;

// Defines the survey's current state.
enum State {
// Default value when status is missing.
STATE_UNSPECIFIED = 0;

// Indicates the survey organizer has not yet completed the survey creation flow.
DRAFT = 1;

// Survey is ready to be used/edited.
READY = 2;
}

// Defines the survey's current state.
State state = 7;
}

// Defines a user's role in a survey.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {of} from 'rxjs';
import {AclEntry} from 'app/models/acl-entry.model';
import {Job} from 'app/models/job.model';
import {Role} from 'app/models/role.model';
import {DataSharingType, Survey} from 'app/models/survey.model';
import {DataSharingType, Survey, SurveyState} from 'app/models/survey.model';
import {Task, TaskType} from 'app/models/task/task.model';
import {AuthService} from 'app/services/auth/auth.service';
import {NavigationService} from 'app/services/navigation/navigation.service';
Expand Down Expand Up @@ -88,7 +88,8 @@ describe('SurveyListComponent', () => {
),
}),
/* acl= */ Map(),
{type: DataSharingType.PRIVATE}
{type: DataSharingType.PRIVATE},
SurveyState.READY
);

const surveyServiceSpy = jasmine.createSpyObj('SurveyService', [
Expand Down
18 changes: 3 additions & 15 deletions web/src/app/components/survey-list/survey-list.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {Component, OnDestroy, OnInit} from '@angular/core';
import {List} from 'immutable';
import {Subscription} from 'rxjs';

import {Survey} from 'app/models/survey.model';
import {Survey, SurveyState} from 'app/models/survey.model';
import {NavigationService} from 'app/services/navigation/navigation.service';
import {SurveyService} from 'app/services/survey/survey.service';

Expand Down Expand Up @@ -62,6 +62,7 @@ export class SurveyListComponent implements OnInit, OnDestroy {
onNewSurvey() {
this.navigationService.navigateToCreateSurvey(null);
}

/**
* Clean up Rx subscription when cleaning up the component.
*/
Expand All @@ -70,19 +71,6 @@ export class SurveyListComponent implements OnInit, OnDestroy {
}

private isSetupFinished(survey: Survey): boolean {
// To make it simple we are not checking the LOIs here since defining tasks is the step after defining LOIs.
return this.hasTitle(survey) && this.hasJob(survey) && this.hasTask(survey);
}

private hasTitle(survey: Survey): boolean {
return survey.title.trim().length > 0;
}

private hasJob(survey: Survey): boolean {
return survey.jobs.size > 0;
}

private hasTask(survey: Survey): boolean {
return survey.jobs.valueSeq().some(job => (job.tasks?.size || 0) > 0);
return survey.state === SurveyState.READY;
}
}
2 changes: 1 addition & 1 deletion web/src/app/converters/firebase-data-converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import {DocumentData, Timestamp} from '@angular/fire/firestore';
import {List, Map} from 'immutable';
import {Map} from 'immutable';

import {AuditInfo} from 'app/models/audit-info.model';
import {Job} from 'app/models/job.model';
Expand Down
29 changes: 22 additions & 7 deletions web/src/app/converters/proto-model-converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {List, Map} from 'immutable';

import {Job} from 'app/models/job.model';
import {Role} from 'app/models/role.model';
import {DataSharingType} from 'app/models/survey.model';
import {DataSharingType, SurveyState} from 'app/models/survey.model';
import {
Cardinality,
MultipleChoice,
Expand All @@ -38,6 +38,11 @@ const PB_ROLES = Map([
[Role.VIEWER, Pb.Role.VIEWER],
]);

const PB_STATES = Map([
[SurveyState.DRAFT, Pb.Survey.State.DRAFT],
[SurveyState.READY, Pb.Survey.State.READY],
]);

/**
* Converts Role instance to its proto message type.
*/
Expand Down Expand Up @@ -72,29 +77,39 @@ export function newSurveyToDocument(
name: string,
description: string,
acl: Map<string, Role>,
ownerId: string
ownerId: string,
state: SurveyState
): DocumentData | Error {
return toDocumentData(
new Pb.Survey({
name,
description,
acl: acl.map(role => roleToProtoRole(role)).toObject(),
ownerId,
state: PB_STATES.get(state),
})
);
}

/**
* Returns the proto representation of a partial Survey model object.
*/
export function partialSurveyToDocument(
name: string,
description?: string
): DocumentData | Error {
export function partialSurveyToDocument({
name,
description,
state,
}: {
name?: string;
description?: string;
state?: SurveyState;
}): DocumentData | Error {
return toDocumentData(
new Pb.Survey({
name,
...(name && {name}),
rfontanarosa marked this conversation as resolved.
Show resolved Hide resolved
...(description && {description}),
...(state && {
state: PB_STATES.get(state),
}),
})
);
}
Expand Down
13 changes: 10 additions & 3 deletions web/src/app/converters/survey-data-converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import {DataCollectionStrategy, Job} from 'app/models/job.model';
import {Role} from 'app/models/role.model';
import {DataSharingType, Survey} from 'app/models/survey.model';
import {DataSharingType, Survey, SurveyState} from 'app/models/survey.model';
import {
Cardinality,
MultipleChoice,
Expand Down Expand Up @@ -73,6 +73,11 @@
[Pb.Survey.DataSharingTerms.Type.CUSTOM, DataSharingType.CUSTOM],
]);

const MODEL_STATES = Map([
[Pb.Survey.State.DRAFT, SurveyState.DRAFT],
[Pb.Survey.State.READY, SurveyState.READY],
]);

function dataSharingTypeFromProto(
protoType?: Pb.Survey.DataSharingTerms.Type | null
) {
Expand Down Expand Up @@ -230,7 +235,8 @@
{
type: dataSharingTypeFromProto(pb.dataSharingTerms?.type),
customText: pb.dataSharingTerms?.customText ?? undefined,
}
},
MODEL_STATES.get(pb.state)
);
}

Expand Down Expand Up @@ -262,7 +268,8 @@
{
type: dataSharingTypeFromProto(data.dataSharingTerms.type),
customText: data.dataSharingTerms.customText,
}
},
data.state
);
}

Expand Down Expand Up @@ -384,7 +391,7 @@
return new TaskCondition(
map?.matchType,
List(
map?.expressions?.map((expression: any) => ({

Check warning on line 394 in web/src/app/converters/survey-data-converter.ts

View workflow job for this annotation

GitHub Actions / Check

Unexpected any. Specify a different type
expressionType: expression.expressionType,
taskId: expression.taskId,
optionIds: List(expression.optionIds),
Expand Down
23 changes: 11 additions & 12 deletions web/src/app/models/survey.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ export enum DataSharingType {
CUSTOM = 3,
}

/** Enum for survey's current state. */
export enum SurveyState {
UNSAVED = 0,
DRAFT = 1,
READY = 2,
}

export class Survey extends Copiable {
static readonly UNSAVED_NEW = new Survey(
/* id= */
Expand All @@ -40,7 +47,8 @@ export class Survey extends Copiable {
/* acl= */
Map<string, Role>(),
/* dataSharingTerms= */
{type: DataSharingType.PRIVATE}
{type: DataSharingType.PRIVATE},
SurveyState.UNSAVED
);

constructor(
Expand All @@ -49,7 +57,8 @@ export class Survey extends Copiable {
readonly description: string,
readonly jobs: Map<string, Job>,
readonly acl: Map<string, Role>,
readonly dataSharingTerms: {type: DataSharingType; customText?: string}
readonly dataSharingTerms: {type: DataSharingType; customText?: string},
readonly state?: SurveyState
) {
super();
}
Expand All @@ -58,16 +67,6 @@ export class Survey extends Copiable {
return this.jobs.get(jobId);
}

isUnsavedNew() {
return (
!this.id &&
!this.title &&
!this.description &&
!this.jobs.size &&
!this.acl.size
);
}

getJobsSorted(): List<Job> {
return this.jobs.sortBy(job => job.index).toList();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ import {ActivatedRoute} from '@angular/router';
import {List, Map} from 'immutable';
import {Observable, Subject} from 'rxjs';

import {DataCollectionStrategy, Job} from 'app/models/job.model';
import {Job} from 'app/models/job.model';
import {LocationOfInterest} from 'app/models/loi.model';
import {DataSharingType, Survey} from 'app/models/survey.model';
import {DataSharingType, Survey, SurveyState} from 'app/models/survey.model';
import {Task, TaskType} from 'app/models/task/task.model';
import {
CreateSurveyComponent,
Expand Down Expand Up @@ -118,7 +118,8 @@ describe('CreateSurveyComponent', () => {
job001: jobWithTask,
}),
/* acl= */ Map(),
{type: DataSharingType.PRIVATE}
{type: DataSharingType.PRIVATE},
SurveyState.READY
);
beforeEach(waitForAsync(() => {
navigationServiceSpy = jasmine.createSpyObj<NavigationService>(
Expand Down
31 changes: 14 additions & 17 deletions web/src/app/pages/create-survey/create-survey.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {filter, first, firstValueFrom} from 'rxjs';

import {Job} from 'app/models/job.model';
import {LocationOfInterest} from 'app/models/loi.model';
import {Survey} from 'app/models/survey.model';
import {Survey, SurveyState} from 'app/models/survey.model';
import {DataSharingTermsComponent} from 'app/pages/create-survey/data-sharing-terms/data-sharing-terms.component';
import {JobDetailsComponent} from 'app/pages/create-survey/job-details/job-details.component';
import {SurveyDetailsComponent} from 'app/pages/create-survey/survey-details/survey-details.component';
Expand Down Expand Up @@ -100,15 +100,14 @@ export class CreateSurveyComponent implements OnInit {
}

private isSetupFinished(survey: Survey): boolean {
// To make it simple we are not checking the LOIs here since defining tasks is the step after defining LOIs.
return this.hasTitle(survey) && this.hasJob(survey) && this.hasTask(survey);
return survey.state === SurveyState.READY;
}

private getSetupPhase(
survey: Survey,
lois: Immutable.List<LocationOfInterest>
): SetupPhase {
if (this.hasTask(survey)) {
if (survey.state === SurveyState.READY) {
return SetupPhase.DEFINE_DATA_SHARING_TERMS;
}
if (!lois.isEmpty()) {
Expand All @@ -127,14 +126,6 @@ export class CreateSurveyComponent implements OnInit {
return survey.title.trim().length > 0;
}

private hasJob(survey: Survey): boolean {
return survey.jobs.size > 0;
}

private hasTask(survey: Survey): boolean {
return survey.jobs.valueSeq().some(job => (job.tasks?.size || 0) > 0);
}

readonly setupPhaseToTitle = new Map<SetupPhase, String>([
[SetupPhase.SURVEY_DETAILS, 'Create survey'],
[SetupPhase.JOB_DETAILS, 'Add a job'],
Expand Down Expand Up @@ -287,13 +278,19 @@ export class CreateSurveyComponent implements OnInit {

private async saveDataSharingTerms() {
const type = this.dataSharingTerms?.formGroup.controls.type.value;

const customText =
this.dataSharingTerms?.formGroup.controls.customText.value ?? undefined;
await this.surveyService.updateDataSharingTerms(
this.survey!.id,
type,
customText
);

await Promise.all([
this.surveyService.updateDataSharingTerms(
this.survey!.id,
type,
customText
),

this.surveyService.updateStatus(this.survey!.id, SurveyState.READY),
]);
}

@ViewChild('surveyLoi')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {MatDialog} from '@angular/material/dialog';
import {Observable, Subscription} from 'rxjs';
import {take} from 'rxjs/operators';

import {Survey} from 'app/models/survey.model';
import {Survey, SurveyState} from 'app/models/survey.model';
import {AuthService} from 'app/services/auth/auth.service';
import {LocationOfInterestService} from 'app/services/loi/loi.service';
import {NavigationService} from 'app/services/navigation/navigation.service';
Expand Down Expand Up @@ -109,9 +109,10 @@ export class MainPageComponent implements OnInit {
this.dialog.open(JobDialogComponent, {
autoFocus: jobId === NavigationService.JOB_ID_NEW,
data: {
surveyId: survey.isUnsavedNew()
? NavigationService.SURVEY_ID_NEW
: survey.id,
surveyId:
survey.state === SurveyState.UNSAVED
? NavigationService.SURVEY_ID_NEW
: survey.id,
createJob: jobId === NavigationService.SURVEY_ID_NEW,
job: survey.jobs?.get(jobId),
},
Expand Down
Loading
Loading