Skip to content

Conversation

@DanBloxham-sw
Copy link
Contributor

@DanBloxham-sw DanBloxham-sw commented Jan 11, 2022

…ments to be a compound key

JIRA link

https://softwiretech.atlassian.net/browse/HEEDLS-712

Description

The CompetencyAssessmentQuestionRoleRequirements table should only have one record for each SelfAssessmentID/CompetencyID combination, but the table did not actually enforce this. As advised by Kevin, I have removed the previous PK on the ID column, and the column itself, and replaced it with a compound PK on SelfAssessmentID and CompetencyID.

I have also made code changes where the ID was used and confirmed the method in question functions appropriately.

Screenshots

N/A


Developer checks

(Leave tasks unticked if they haven't been appropriate for your ticket.)

I have:

  • Run the formatter and made sure there are no IDE errors.
  • Written tests for the changes (accessibility tests, unit tests for controller, data services, services, view models, etc)
  • Manually tested my work with and without JavaScript. Full manual testing guidelines can be found here: https://softwiretech.atlassian.net/wiki/spaces/HEE/pages/6703648740/Testing
  • Updated/added documentation in Swiki and/or Readme. Links (if any) are below:
  • Updated my Jira ticket with information about other parts of the system that were touched as part of the MR and have to be sanity tested to ensure nothing’s broken.
  • Scanned over my own MR to ensure everything is as expected.

using FluentMigrator;

[Migration(202201111021)]
public class ChangeCompetencyAssessmentQuestionRoleRequirementsPrimaryKey: Migration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the original migration made when we wanted to replace the ID column with a compound PK on the two necessary columns. As it was pushed into an MR, and thus run on Jenkins, prior to the decision to just use a unique constraint instead, this migration has been left, and the other migration tackles it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, missed this comment!

Comment on lines +12 to +14
Alter.Table("CompetencyAssessmentQuestionRoleRequirements").AddColumn("ID").AsInt32().NotNullable().Identity();
Create.PrimaryKey("PK_CompetencyAssessmentQuestionRoleRequirements")
.OnTable("CompetencyAssessmentQuestionRoleRequirements").Column("ID");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might notice that this is not identical to the Down() method in the other migration. I only noticed when creating this migration that adding a column with .PrimaryKey() when altering a table (rather than creating) doesn't actually make the column a primary key. This seems to be due to a subtlety in FluentMigrator that should probably be more obvious but isn't, see here for details: fluentmigrator/fluentmigrator#96

Delete.PrimaryKey("PK_CompetencyAssessmentQuestionRoleRequirements")
.FromTable("CompetencyAssessmentQuestionRoleRequirements");
Alter.Table("CompetencyAssessmentQuestionRoleRequirements").AddColumn("ID").AsInt32().NotNullable()
.PrimaryKey().Identity();
Copy link
Contributor

Choose a reason for hiding this comment

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

Given your comment on the next migration, should this Down() have Create.PrimaryKey...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. When I made the other migration, I thought it would be good to leave this one as is for some reason. But now I think it would break anytime someone wanted to migrate down beyond this. I've updated it to include the correct primary key addition.

Copy link
Contributor

@SteveJacksonSoft SteveJacksonSoft 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!

@DanBloxham-sw DanBloxham-sw merged commit 1db8721 into master Jan 12, 2022
@DanBloxham-sw DanBloxham-sw deleted the HEEDLS-712-ChangeCompetencyAssessmentQuestionRoleRequirementsPrimaryKey branch January 12, 2022 16:01
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.

4 participants