feat: added logs to errors imports

eduNEXT / eox-core

eox-core is a plugin to extend the core functionality in Open edX

GNU Affero General Public License v3.0

15 stars 9 forks source link

feat: added logs to errors imports #259

Closed BetoFandino closed 6 months ago

BetoFandino commented 11 months ago

Logs in errors imports

Description

This change is due to the difficulty in tracking errors in some imports. They will help to locate faster any errors or warnings that may occur that may hinder testing and debugging.

Useful information to include:

Which user roles will this change impact: Developer.

Checklist for Merge

[ x] Tested in a remote environment / NA
[x ] Updated documentation / NA
[ x] Rebased master/main / NA
[x ] Squashed commits / NA

MaferMazu commented 11 months ago

Thanks for this @BetoFandino. In #254, we already added social-auth-core as a requirement for this plugin to avoid the try-except errors related to social_core. But we probably still need your proposal for other packages, for example, sentry. Let me know when you require a review for this 🙌

BetoFandino commented 11 months ago

Hi @MaferMazu, super thank you very much but then I ask you two little things:

Can we assume that they are going to apply that for sentry and therefore we close this pr and wait for you? or do you need our help to push that?
I'm not sure if you removed it but that PR you put, if you made social-core mandatory then it would be good to remove this too https://github.com/eduNEXT/eox-core/blob/master/setup.py#L76 ?

MaferMazu commented 11 months ago

You already have this PR @BetoFandino; we can continue the process of adding the try-except to sentry imports in your PR, and in the palm support PR, handle thesocial_core imports. About the TPA requirements, thanks for that, I didn't notice, I will remove it in #254

BetoFandino commented 11 months ago

Ok @MaferMazu, then I will make the pertinent changes in this PR. Thank you very much!!! 😃

MaferMazu commented 10 months ago

@BetoFandino, do you still want to merge this?

BetoFandino commented 10 months ago

@MaferMazu if possible yes please

MaferMazu commented 10 months ago

@BetoFandino, can you fix the issues, please?

bra-i-am commented 8 months ago

hi! I hope you're well.

@BetoFandino, just a friendly reminder about this PR so we can continue the merging process, thank you c:

MaferMazu commented 6 months ago

Thanks for updating this, @BetoFandino. I only have a question: Why do we need the file coursekey_h_v1? In my understanding, that file is to support the Open edX Release Hawthorn, and right now, we have a coursekey_m_v1 that supports Maple and all the next releases.

The rest looks good to me. When we figure out the Hawthorn backend, we will be good to merge.

BetoFandino commented 6 months ago

Hi @MaferMazu , yes you are right coursekey_h_v1 is no longer needed, I removed it and updated the PR.