fix: 500 error during opening program certificate page (master) #4182
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @DmytroAlipov! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
Hi @openedx/2u-phoenix! Would someone be able to please review / merge this for us? |
DawoudSheraz
force-pushed
the
fix-org-image
branch
from
136acc4 to
e99b7bd
Compare
| super().__init__(*args, **kwargs) | ||
| self.fields['certificate_logo_image'].required = True |
There was a problem hiding this comment.
This field is not required necessarily for Discovery's functioning. Discovery's codebase assumes it is None in certain places (like API). It is better to set these expectations in consumer rather than requiring the field here.
There was a problem hiding this comment.
Since organizations are edited via admin, it can impact the editors/admins when they have to add the value even if they dont need it.
There was a problem hiding this comment.
Hi @DawoudSheraz. You’re correct, the certificate_logo_image is not required in the database. However, the proper display of the program certificate page is directly dependent on this field. That’s why I’ve implemented checks for the Programs and also suggest making this field required for Organizations to fill out. What potential risks do you foresee, apart from the fact that administrators might be compelled to add a logo even if they don’t need it?
There was a problem hiding this comment.
What potential risks do you foresee, apart from the fact that administrators might be compelled to add a logo even if they don’t need it?
At this point, the admins getting blocked on the field and having to add a logo (default or otherwise) is the main issue. This is why I suggested having the appropriate checks on the consumer side. Or you can add these changes behind a toggle/switch(?).
DmytroAlipov
force-pushed
the
fix-org-image
branch
from
e99b7bd to
23165f3
Compare
|
Hi @DawoudSheraz! I reconsidered my decision, and I agree with you that there's no need to make the "Certificate logo image" field required. Now this code is not available in all PRs (master, Palm, Quince). |
|
@DawoudSheraz Could you help me with the pipeline problem? I don't understand why test_typeahead_org_course_runs_come_up_first fails. Moreover, it does not work only for the master branch. In the PR version of Quince and Palm, everything is fine. |
DawoudSheraz
force-pushed
the
fix-org-image
branch
from
23165f3 to
206c55f
Compare
| @@ -68,6 +68,21 @@ def clean(self): | |||
|
|
|||
| return self.cleaned_data | |||
|
|
|||
| def clean_authoring_organizations(self): | |||
There was a problem hiding this comment.
nit: add method docstring
There was a problem hiding this comment.
@AfaqShuaib09 done
DmytroAlipov
force-pushed
the
fix-org-image
branch
from
206c55f to
069c79b
Compare
Error 500 occurs when going to the program certificate page if an organization is connected to the Authoring organizations field with an empty field Certificate logo image.
|
@DmytroAlipov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
Error 500 occurs when going to the Program certificate page if an organization is connected to the
Authoring organizationsfield with an empty fieldCertificate logo image:Steps to Reproduce:
Authoring organizationsfield that does not have an image loaded for theCertificate logo image field:I propose that before saving the program, verify if the Certificate logo image field of the associated organizations is empty. Also, make the certificate_logo_image field required to be filled in the admin panel, but not in the model, to prevent unnecessary migrations.
On the Program page
https://<discovery>/admin/course_metadata/program/:On the Organization page
https://<discovery>/admin/course_metadata/organization/: