Extract methods from experience-cs #87
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've extracted this method out of experience-cs [1]. This seems like a more logical home for it and we could avoid the duplication in editor-api [2] and possibly other apps. Since you have to explicitly include the new concern into the `User` model in your app, this should be a non-breaking change. [1]: https://github.com/RaspberryPiFoundation/experience-cs/blob/837bf97d4aec7ed915f87f9dd9253b87e2dd00b0/app/models/user.rb#L15-L17 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/ecb784299eea43654660111aa509eedc8270346d/app/models/user.rb#L61-L63
I've extracted this method out of experience-cs [1]. This seems like a more logical home for it and it might be useful in other apps. There's some related functionality in editor-api for removing the "student:" prefix from the user's ID which might make sense to extract into here too [2], especially if we end up needing to do the same thing in experience-cs which seems likely. Since you have to explicitly include the new concern into the `User` model in your app, this should be a non-breaking change. [1]: https://github.com/RaspberryPiFoundation/experience-cs/blob/837bf97d4aec7ed915f87f9dd9253b87e2dd00b0/app/models/user.rb#L19-L21 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/ecb784299eea43654660111aa509eedc8270346d/app/models/user.rb#L109
I'm about to make some changes which will affect this, but it hasn't
been updated in quite a while, so I thought I would update it first so
it was clearer what I was changing.
I ran the following command:
bundle exec rubocop --auto-gen-config
Previously we were defining a `DummyUser` class in each of these specs and including just the concerns that were needed for the examples. However, this `DummyUser` class is effectively global and so there was a possibility of cross-contamination. I could have improved matters by choosing a different class name in each spec. However, I think it's better to dynamically create a class in each spec. A side benefit is that I've managed to fix the four RSpec/SpecFilePathFormat violations, so I've re-generated the rubocop to-do list again and incorporated the changes into this commit.
Test coverage: 100.0% |
sebjacobs
approved these changes
Jun 4, 2025
There was a problem hiding this comment.
❤️ thanks for doing this @floehopper. It feels much better having this code here.
floehopper
added a commit
that referenced
this pull request
Jun 4, 2025
Since this adds new functionality in a backward compatible manner, it makes sense for it to be a minor version bump from v4.1.1 -> v4.2.0. I've split #87 into two lines in the CHANGELOG for clarity.
floehopper
added a commit
that referenced
this pull request
Jun 4, 2025
Since this adds new functionality in a backward compatible manner, it makes sense for it to be a minor version bump from v4.1.1 -> v4.2.0. I've split #87 into two lines in the CHANGELOG for clarity. I've only included the version change of them gem itself in the Gemfile changes; I discarded changes in versions of other gems which just seemed to be because the dependencies are unconstrained and they are not needed for the changes in this release.
floehopper
added a commit
that referenced
this pull request
Jun 4, 2025
Since this adds new functionality in a backward compatible manner, it makes sense for it to be a minor version bump from v4.1.1 -> v4.2.0. I've split #87 into two lines in the CHANGELOG for clarity. I've only included the version change of them gem itself in the Gemfile changes; I discarded changes in versions of other gems which just seemed to be because the dependencies are unconstrained and they are not needed for the changes in this release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This has come out of https://github.com/RaspberryPiFoundation/experience-cs/issues/546.
This adds two new optional model concerns which provide the following methods (see individual commits for details):
I've also made a small improvement to the model concern specs and fixed some rubocop violations.
I've tested the changes in https://github.com/RaspberryPiFoundation/experience-cs/pull/687.