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

refactor: REST API to return a different response if LR MFE is enabled #1747

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

MaxFrank13
Copy link
Member

This PR refactors a REST API endpoint that creates a ProgramCertRecord and returns the URL for that public record. Currently, that URL is routing to the legacy frontend. With this refactor, if the Learner Record MFE is enabled, it will instead return the URL for the MFE.

@MaxFrank13 MaxFrank13 force-pushed the mfrank/LR-MFE-public-link branch from 5174eaa to b539e61 Compare September 7, 2022 18:05
@MaxFrank13 MaxFrank13 force-pushed the mfrank/LR-MFE-public-link branch from b539e61 to 4f3efc4 Compare September 7, 2022 18:56
response = self.client.post(rev, data=jdata, content_type=JSON_CONTENT_TYPE)
json_data = response.json()
self.assertEqual(response.status_code, 201)
self.assertRegex(json_data["url"], UUID_PATTERN)
Copy link
Contributor

Choose a reason for hiding this comment

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

One last suggestion. Can we improve the verification of this unit test? Instead of checking if the UUID is contained within the string, can we verify that the entire string is being built as we expected?

Since we are are now building a link to an external system... this would be a stronger test if we verified that the link is exactly as we expect it to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like another job for a format string!

Copy link
Contributor

@justinhynes justinhynes left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for being open to the suggestions I posed. :)

@MaxFrank13 MaxFrank13 force-pushed the mfrank/LR-MFE-public-link branch from 4f3efc4 to 530055a Compare September 7, 2022 19:44
@MaxFrank13 MaxFrank13 force-pushed the mfrank/LR-MFE-public-link branch from 530055a to f84d7b6 Compare September 7, 2022 19:49
@MaxFrank13 MaxFrank13 merged commit 8a57c46 into master Sep 7, 2022
@MaxFrank13 MaxFrank13 deleted the mfrank/LR-MFE-public-link branch September 7, 2022 20:11
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.

2 participants