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.

Sign up for GitHub

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

Jump to bottom

Create LtiIntegration model #51734

Merged
merged 7 commits into from
May 18, 2023
Merged

Create LtiIntegration model #51734

merged 7 commits into from
May 18, 2023

Conversation

nicklathe
Copy link
Contributor

@nicklathe nicklathe commented May 5, 2023 •
edited

This PR adds a new LtiIntegration model. I'm looking for feedback on any standards we have that I'm violating, or best practices I've missed. It includes:

  • Model definition
  • Migration
  • Schema change
  • Tests
  • Updates to factories.rb

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@nicklathe nicklathe self-assigned this May 5, 2023
@nicklathe nicklathe requested a review from a team as a code owner May 5, 2023 16:51
@nicklathe nicklathe marked this pull request as draft May 5, 2023 16:51
@nicklathe nicklathe removed the request for review from a team May 5, 2023 16:51
@nicklathe nicklathe closed this May 5, 2023
@nicklathe nicklathe reopened this May 5, 2023
@nicklathe nicklathe marked this pull request as ready for review May 9, 2023 23:10
@nicklathe nicklathe requested review from a team May 9, 2023 23:10
@nicklathe nicklathe changed the title Draft: Initialize LtiIntegration model Initialize LtiIntegration model May 9, 2023
@nicklathe nicklathe changed the title Initialize LtiIntegration model Create LtiIntegration model May 9, 2023
@nicklathe
Initialize LtiIntegration model
b81d482 
Signed-off-by: Nick Lathe <nick.lathe@code.org>
@nicklathe
Update LtiIntegration model
2972121 
  - Add tests
  - Add validations
  - Remove generated lti_integration_factories.rb
  - Run rails db:miigrate
@nicklathe
Use factories to set up default LtiIntegration object
b202de3
@nicklathe nicklathe force-pushed the nicklathe/p20-101-lti-integrations branch from 06d8ca9 to b202de3 Compare May 10, 2023 16:04
@sureshc
Copy link
Contributor

sureshc commented May 11, 2023 •
edited

Because it’s hard to revert a Rails database/schema migration, we have an undocumented practice of putting the actual schema change (the migration and the new schema.rb) in its own Pull Request and merging that before any feature branches that implement validations, tests, load fixtures, etc. This reduces risks if the migration causes issues and needs to be rolled back somehow. Sorry this is not documented anywhere! We should write up some ActiveRecord best practices and/or lint rules to better guide the team on this.

@nicklathe
Copy link
Contributor Author

Because it’s hard to revert a Rails database/schema migration, we have an undocumented practice of putting the actual schema change (the migration and the new schema.rb) in its own Pull Request and merging that before any feature branches that implement validations, tests, load fixtures, etc. This reduces risks if the migration causes issues and needs to be rolled back somehow. Sorry this is not documented anywhere! We should write up some ActiveRecord best practices and/or lint rules to better guide the team on this.

Ok, this documentation exists. I must have misread it as this PR should include the model changes and any other files generated from rails generate migration....

Will update PR to include only the schema.rb and the 20230503220558_create_lti_integrations.rb files.

@nicklathe nicklathe mentioned this pull request May 15, 2023
Merged
8 tasks
@nicklathe
Copy link
Contributor Author

@sureshc here is a separate PR that adds only the migration and schema. Once that is approved and merged, I will rebase this PR to add the additional model, tests, and factory changes.

#51889

@nicklathe nicklathe marked this pull request as draft May 15, 2023 18:01
@nicklathe nicklathe marked this pull request as ready for review May 16, 2023 18:49
cat5inthecradle
cat5inthecradle reviewed May 16, 2023
View reviewed changes
dashboard/test/factories/factories.rb Outdated
Comment on lines 1684 to 1692
factory :lti_integration do
platform_id {"1a2b3c5b"}
issuer {"http://some.lms.com"}
client_id {"12345678"}
platform_name {"canavs"}
auth_redirect_url {"http://some.lms.com/lti/authenticate"}
jwks_url {"http://some.lms.com/jwks"}
access_token_url {"http://some.lms.com/access_token"}
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we tend to use hardcoded values in factories, or do we ever prefer randomization? I honestly don't know. I'm a big fan of random so we discover edge cases earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I'll take a look at some other factories for inspiration.

Copy link
Contributor Author

@nicklathe nicklathe May 17, 2023 •
edited

Choose a reason for hiding this comment

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

It seems like a lot of values in factories are hardcoded. However I've changed these to just be string values named as their key. If there is a preferred standard, I'm happy to follow it.

elf-code
elf-code approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@elf-code elf-code left a comment

Choose a reason for hiding this comment

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

LGTM, left one nit on test language.

dashboard/test/models/lti_integration_test.rb Outdated Show resolved Hide resolved
@nicklathe nicklathe merged commit f567185 into staging May 18, 2023
2 checks passed
@nicklathe nicklathe deleted the nicklathe/p20-101-lti-integrations branch May 18, 2023 22:01
@elf-code elf-code mentioned this pull request May 24, 2023
Merged
8 tasks
pablo-code-org pushed a commit that referenced this pull request May 25, 2023
@nicklathe @pablo-code-org
Create LtiIntegration model (#51734)
b8a90f2 
Initialize LTI Integration model

  - Add validations
  - Add tests

Signed-off-by: Nick Lathe <nick.lathe@code.org
snickell pushed a commit that referenced this pull request Feb 3, 2024
@nicklathe @pablo-code-org
Create LtiIntegration model (#51734)
4870bf5 
Initialize LTI Integration model

  - Add validations
  - Add tests

Signed-off-by: Nick Lathe <nick.lathe@code.org
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cat5inthecradle cat5inthecradle cat5inthecradle approved these changes

@elf-code elf-code elf-code approved these changes

Assignees

@nicklathe nicklathe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicklathe @sureshc @cat5inthecradle @elf-code