-
Notifications
You must be signed in to change notification settings - Fork 147
feat: extend the account fields with the already existing extended_profile_fields #1193
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
Conversation
Thanks for the pull request, @bra-i-am! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1193 +/- ##
==========================================
+ Coverage 58.55% 60.31% +1.76%
==========================================
Files 119 121 +2
Lines 2413 2505 +92
Branches 665 692 +27
==========================================
+ Hits 1413 1511 +98
+ Misses 934 931 -3
+ Partials 66 63 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@bra-i-am looks like this PR introduces some code coverage issues, can you please have a look? |
191fca6
to
9301a87
Compare
97fba46
to
946eb1b
Compare
@e0d, I tried to cover all the code lines required to achieve the codecov check; there are some more warnings, but many are in lines so specific that I didn't get them with my changes. Please let me know if covering the whole code is required or if having the codecov check is enough. Thanks a lot! 🙏 |
946eb1b
to
e3f6977
Compare
e3f6977
to
9054b03
Compare
@awais-ansari @sundasnoreen12, I hope you are doing great! I've been considering that this merge request might encounter issues during the merge due to the incorporation of a new feature. However, I also believe it would serve as a good recovery from the features lost in the transition to the MFEs (I planted this solution motivated by https://discuss.openedx.org/t/custom-fields-in-profile-form/6116/2, indeed). In any case, I'm waiting for your comments to plan how I could proceed. Beforehand, thank you so much for your time! |
|
||
import messages from './AccountSettingsPage.messages'; | ||
|
||
const ExtendedProfileField = (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I not sure if this is necessary at the end is a wrapper, why not instead being a section in the account page (something like profile extra information or extension) or a subsection in the profile information section (as is shown in your implementation) that given the extended_profile information, render the corresponding fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it only to avoid the component AccountSettingsPage
from being bigger than it is at this moment. Additionally, maybe in the future we would add more field.type
options, which could make the switch statement increase in size
Do you think it makes sense? :s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the AccountSettingsPage
, should not be larger, and if you are going to make this a slot extension I think having a section that group all the logic is better, what that means:
<AccountSettingsPage>
....
<ExtendedProfile extendedData={extendedFilds}>
....
</AccountSettingsPage>
And then inside the component make the map, validate the data type and return the components needed
…ame saga function
…ering in EditableCheckboxField
0f16067
to
b41b11e
Compare
@dcoa @awais-ansari @sundasnoreen12, I hope you are doing well I'm closing this PR in favor of #1254, which creates a plugin slot to improve the handling of this thanks ✨ |
Description
This PR aims to allow the extension of the default account fields by using the
extended_profile_fields
feature with a custom form app like the exampleMain changes
extended_profile_field
settings attribute (it is only covered checkbox, text and select fields)mfe_context
API)How Has This Been Tested?
Sumac
environmenttutor mounts add /path/to/cloned/mfe
tutor mounts add lms:/path/to/cloned/custom-form-app:/openedx/extra-deps/custom-form-app
and install it usingtutor dev exec lms pip install -e ../extra-deps/custom-form-app
Site Configurations > local.openedx.io:8000
:tutor dev restart
Screenshots/sandbox (optional):