Development Standards, Decisions, and Troubleshooting - bcgov/SIMS GitHub Wiki

This page is intended to define decisions about code, patterns and whatever else that represents what the Teams must try their best to follow.

To view the DevOps documentation, go here.

• OpenShift
• Keyclock
• Docker (local development setup - you will need to include node.js & npm)
• Make/Test CMD

• Volar
• TypeScript Vue Plugin (Volar)
• ESLint
• Prettier - Code formatter
• Code Spell Checker
• Better Comments (optional but recommended)
• JavaScript Debugger (Nightly)
• SQL Formatter
• YAML - Plugin to format YAML files

• Create a new file in VS code and check the format. It must be LF as shown here.

If otherwise please make sure your VS code file editor settings are configured to use LF.

• Check for core.autocrlf settings in GIT. Run the following command and it must return false. git config --get core.autocrlf

If the above command returns true, please run the following command

git config --local core.autocrlf false

Ref: https://stackoverflow.com/questions/1967370/git-replacing-lf-with-crlf

Now validate again with git config --get core.autocrlf to ensure your command ran successfully. The command must return false.

The GitHub repository contains the VS Code launch.json that will enable the debug option as shown below.

1. Start the API using npm run start:debug. This command is not executing the migrations. If needed the migrations can be executed also using npm run setub:db.
2. Navigate to VSCode debug menu and attach the debug to the process.

/**
 * This is my const.
 */
const THIS_IS_A_CONST = 123456;

/**
 * My comment with a proper sentence that starts usually with a
 * capital letter and finishes with a period.
 * Try to add comments that make it better to understand the
 * business logic does not only describes the method itself.
 */
export class MyClassWithACRONYM {
  /**
   * My private local variable.
   */
  private myLocalVariable = 123;

  constructor(private readonly myVariable: string) {
    // Do something
  }

  /**
   * Try to give a comment that provides some business context instead
   * of only describing what we can read from the code itself.
   * For instance, for the below 'if' condition, instead of saying
   * 'If THIS_IS_A_CONST is equal to myNumber then do something we can explain why
   * do we need to check it from a business perspective.
   * @param myNumber my number that we need for this and that,
   * @param myOptionalParameter used in the case A and B when X is needed.
   */
  async myPublicMethod(
    myNumber: number,
    myOptionalParameter?: string
  ): Promise<number> {
    if (myNumber === THIS_IS_A_CONST) {
      // Use early return when possible to reduce code complexity.
      return this.myAsyncMethodCallA(myOptionalParameter);
    }

    return this.myAsyncMethodCallB(this.myVariable);
  }

  /**
   * Add nice comments as described before.
   */
  private myPrivateMethod() {
    // Do something.
  }
}

/**
 * Enumeration types for Notes.
 */
export enum NoteType {
  /**
   * Note type general.
   */
  General = "General",
  /**
   * Note type Restriction.
   */
  Restriction = "Restriction",
  /**
   * Note type System.
   */
  System = "System actions",
  /**
   * Note type Program.
   */
  Program = "Program",
}

/**
 * Entity for notes.
 */
@Entity({ name: TableNames.Notes })
export class Note extends RecordDataModel {
  @PrimaryGeneratedColumn()
  id: number;
  /**
   * Note type.
   */
  @Column({
    name: "note_type",
    type: "enum",
    nullable: false,
  })
  noteType: NoteType;
  /**
   * Description of the note.
   */
  @Column({
    name: "description",
    nullable: false,
  })
  description: string;
/**
   * Total income value retrieved from CRA file
   * (currently line 15000 form tax file).
   */
  @Column({
    name: "cra_reported_income",
    nullable: true,
  })
  craReportedIncome?: number;
}

The dynamic router can conflict with its similar pattern static router, for eg: @Get("programs") can conflict with @Get(":id"), so when creating a dynamic router controller, make sure the similar pattern static controller is at the top and dynamic controller are at the bottom.

ref: https://stackoverflow.com/questions/58707933/node-js-express-route-conflict-issue , https://poopcode.com/how-to-resolve-parameterized-route-conficts-in-express-js/

• DTOs are placed on files with the suffix dto.ts;
• DTOs received by the API are named MyClassAPIInDTO where APIInDTO states that this is a DTO that serves as API input;
• DTOs returned by the API are named MyClassAPIOutDTO where APIOutDTO states that this is a DTO that is returning data from the API;
• The DTO names should be kept the same when mapped on the client application;
• All data returned from the API should be mapped to a DTO and then returned. Do not expose data in the API directly from Business Services Layer or Repository Layer;
• All DTOs must be defined as classes to allow the Swagger documentation to be generated with the automatically out-of-box features as much as possible;
• All DTOs received from the API should have class validators associated with its properties. The only exception is when a form.io Dry Run is performed.

Sample Input DTO

export class MySampleClassAPIInDTO {
  @IsPositive()
  id: number;
  @IsDate()
  myPropertyOne: Date;
  @IsOptional()
  myPropertyTwo: string;
  @ArrayMinSize(1)
  @ValidateNested({ each: true })
  @Type(() => MyNestedObject)
  someArray: MyNestedObject[];
}

Sample Output DTO

export class MySampleClassAPIOutDTO {
  id: number;
  myPropertyOne: Date;
  myPropertyTwo: string;
}

The naming of controller APIs should be in line with the pattern of tag definition in swagger.

Swagger Tag resource-subresource

URN resource/subresource/{pathParameter}/{subresource}

E.g Swagger Tag students-assessment

URN /students/assessment/{assessmentId}/noa /students/assessment/{assessmentId}/award

1. Resources should always be collections (except abbreviations like aest) and not singletons as seen in the example above.
2. Resources and subresources should always be in lowercase.
3. Complex resource or subresource names should be separated with a hyphen character.
4. Path parameters should always be camelcase e.g assessmentId.

Try to have the exception typed with unknown. More information on unknown on catch Clause Bindings

try {
  // Do something that can throw an exception.
} catch (error: unknown) {
  if (error instanceof ApiProcessError) {
    // Do something with the typed object.
    if (error.errorType === SOME_SPECIFIC_ERROR) {
      // For instance, process the specific error.
    }
    return;
  }
  // Do something else.
}

When catching an error and throwing a higher lever error, add a cause. This way the line where the origin was thrown will be logged as well.

    try {
      // Do something...
    } catch (error: unknown) {
      throw new Error(
        "Error while doing xyz.",
        { cause: error },
      );
    }

• When calling an API from the vue, to add the client root, call addClientRoot, as shown below.

If the client type is aest, then the addClientRoot will translate the URL as api/aest/supporting-user/application/${applicationId}

• PR title must follow the pattern "#<Ticket_number> - Nice Description (#)".
• Add appropriate labels that also indicate the applications being impacted by the PR, for instance, SIMS Api or Queue-Consumers.
• Connect the issue using the button "Connect Issue", if not available install the Chrome Extension ZenHub for GitHub or similar.
• If you are the author of the PR avoid resolving the comments, instead reply to them to let the reviewer be aware of your thoughts.
• If you are a reviewer try to mark the comments as resolved to help the PR author to identify easily what is still missing.
• Comments and conversations on the PR are good to let everyone be aware of what is happening but a quick call can also save a lot of time.
• Even if the ticket demands too many file changes, try to create small PRs to make it easier for reviewers to understand the changes. It also makes the PR review quicker.
• Once a review is raised, three reviewers are expected to be assigned to provide feedback and approve it.
  • The developers should take turns associating themselves across the PR and trying to balance the workload.
  • Ideally, 3 developers should be associated with the PR within an hour after it was raised and posted in the DEV Teams chat. The commitment is not to have the review finalized within one hour, it is about having developers associated with the review.
  • The team, as it is right now, will have two main reviewers assigned.
  • The 3 reviewers group must contain at least one main reviewer plus 2 developers.
  • The main reviewers should do the review after the other 2 reviewers provided the feedback. The idea is that the review level achieves a point where we have more main reviewers or a main reviewer is no longer needed.
  • The first reviewer should do the best effort to try to find a good moment to start in the next 3 business hours. It does not mean finishing it in the 3 hours, just try to start providing some feedback. If multiple PRs are open at the same time the delays will be completely acceptable. The goal is to allow the PR owner to start working on possible modifications sooner rather than later.
• PRs are about code review (not people review).
• Delete the branch after the merge is done (after merging do not reuse the branch).

The rollback scripts must be tested prior to PR creation.

1. Navigate to the backend folder.
2. Execute the npm run migration:revert as many times as needed to test all migrations added on the current PR.
3. Execute the npm run migration:run to have all migration applied again.

• Comments must be present for every column.

Right now we are not targeting and testing the solutions (Student, Ministry, etc.) to be responsive at mobile level but we are committed to use as much as possible the out-of-box features available in the UI frameworks currently used. That means that we are not putting efforts towards mobile enablement other than what we are getting out-of-box by just using the modern UI frameworks.

• Download Camnuda modeler

https://camunda.com/download/modeler/

• Use Camelcase;
• Create PR with changed BPMNs and DMNs;
• Use this Git repo for the latest copy;
• Do not hardcode environment-specific data. Use Env variables for environment-specific data;

• Artifactory Reference: https://github.com/BCDevOps/developer-experience/blob/master/apps/artifactory/DEVHUB-README.md#docker-images
• Example pull from artifactory

docker pull artifacts.developer.gov.bc.ca/redhat-docker-remote/ubi8/nodejs-16:1

• Example pull from redhat

docker pull registry.access.redhat.com/ubi8/nodejs-16:1

• Example to see repo's in artifactory

curl -u username:password -X GET "https://artifacts.developer.gov.bc.ca/artifactory/api/repositories?type=remote" | jq -r '(["ARTIFACTORYKEY","SOURCEURL"] | (., map(length*"-"))), (.[] | [.key, .url]) | @tsv' | column -t

• Example to login to artifactory

docker login -u <USER_NAME> -p <USER_PASSWORD> artifacts.developer.gov.bc.ca/<REPO_NAME>

If the credentials to use the artifactory artifacts.developer.gov.bc.ca are updated, then the builds will fail to pull the docker images from the artifactory.

Executing the following steps will update the openshift secret to have the most updated credentials to connect the artifactory.

STEP 1(Find the source) Find the most updated docker config secret present in the tools namespace. This is the source to copy the credentials.

STEP 2(Update the referenced secret): Use the values from the secret found in STEP 1 and update the values in the secret artifactory-secret-credential Note: artifactory-secret-credential is the secret used by our build yml to connect artifactory.

To update the values in the secret use Edit Secret option.

Please be careful NOT to update the source secret from where the credentials were copied from

Step 1: Drag and drop the given component into the form

Step 2: Select calendar picker from the widget options in the display tab

Step 3: Use the following JSON for the widget settings.

{
  "type": "calendar",
  "allowInput": true,
  "clickOpens": true,
  "enableDate": true,
  "enableTime": false,
  "mode": "single",
  "noCalendar": false,
  "format": "yyyy-MM-dd",
  "dateFormat": "yyyy-MM-dd",
  "useLocaleSettings": false,
  "hourIncrement": 1,
  "minuteIncrement": 5,
  "time_24hr": false,
  "saveAs": "text",
  "locale": "en"
}

async getResponseFilesFullPath(
    remoteDownloadFolder: string,
    fileRegexSearch: RegExp,
    recursiveSearch = false,
  ): Promise<string[]> {
    let filesToProcess: Client.FileInfo[];
    const client = await this.getClient();
    try {
      if (recursiveSearch) {
        return await this.getFilePathsRecursively(
          client,
          remoteDownloadFolder,
          fileRegexSearch,
        );
      }
      filesToProcess = await client.list(remoteDownloadFolder, fileRegexSearch);
    } finally {
      await SshService.closeQuietly(client);
    }

    return filesToProcess
      .map((file) => path.join(remoteDownloadFolder, file.name))
      .sort();
  }

  private async getFilePathsRecursively(
    client: Client,
    remoteDownloadFolder: string,
    fileRegexSearch: RegExp,
  ) {
    const filePaths: string[] = [];
    const files = await client.list(remoteDownloadFolder);
    for (const file of files) {
      if (file.type === "d") {
        const subDirectoryFiles = await this.getFilePathsRecursively(
          client,
          path.join(remoteDownloadFolder, file.name),
          fileRegexSearch,
        );
        filePaths.push(...subDirectoryFiles);
      } else {
        console.log(path.join(remoteDownloadFolder, file.name));
        filePaths.push(path.join(remoteDownloadFolder, file.name));
      }
    }
    return filePaths.filter((filePath) =>
      fileRegexSearch.test(filePath.replace(/\\/g, "/")),
    );
  }

When hidden fields have values that must be preserved or enforced during server validations(dryRun), please ensure the below.

• Ensure the hidden fields in added to the form before the input consuming its calculated value.
• persistent should be set to true only if the value needs to be persisted, otherwise setting it to false allows the validation to happen and present the unwanted value to be persisted to DB.
• calculateServer must be true to ensure the calculated value will be reevaluated on the server side preventing user manipulation.
• calculateValue should contain the main logic that will define the input value. See the below image for an example of a constant value that we must ensure will not be changed by malicious user input.