Errors unless FORUM_MONGODB_DATABASE and FORUM_MONGODB_CLIENT_PARAMETERS are defined

[ormsbee]

By default, we had intended for sites running master to require no changes at all to their configuration to keep the old experience. We currently break this, as @regisb notes in this post. The code is currently doing this because we sometimes need to derive the course_id to know which code path to take, and that's not always available from the request itself.

Is it feasible to add checking to forum.backends.mongodb.api.get_course_id_by_thread_id to explicitly check to see if FORUM_MONGODB_DATABASE and FORUM_MONGODB_CLIENT_PARAMETERS exist, and return None if they don't? That should make the public api call get_course_id_by_thread return None. (And the same for get_course_id_by_comment.) When passing a course_id of None to the CourseWaffleFlag check, it should return the default value for the site as a whole. Which I think will do what we want in a backwards compatible way?

Would that work? Are there other places where we need to try to infer the course_id?

[ormsbee]

FYI @taimoor-ahmed-1, @Ali-Salman29, @Faraz32123

[regisb]

Is it feasible to add checking to forum.backends.mongodb.api.get_course_id_by_thread_id to explicitly check to see if FORUM_MONGODB_DATABASE and FORUM_MONGODB_CLIENT_PARAMETERS exist, and return None if they don't?

There are two issues with this approach:

1. These settings have default values which are not None. We could remove those default values, but then that would mean more work for people enabling forum v2.
2. There are some cases when we need get_course_id_by_thread_id to not return None: that is, when forum v1 is enabled across the platform, but forum v2 is enabled only for select courses.

Are there other places where we need to try to infer the course_id?

I believe these are the only two places, but @Faraz32123 might know better.

IMHO, the "right" fix would be to have the forum send the course ID along with the thread ID/comment ID in these HTTP requests. But this is no longer a trivial fix.

[Faraz32123]

I believe these are the only two places, but @Faraz32123 might know better.

Yes, these are the 2 places in forumV2 where we can return None by checking settings. But This will result in above 2 issues. Even if we add this settings check in edx-platform, issue 2 will still remain when forum v2 is enabled only for selected courses.

IMHO, the "right" fix would be to have the forum send the course ID along with the thread ID/comment ID in these HTTP requests. But this is no longer a trivial fix.

Exactly.

[ormsbee]

1. These settings have default values which are not None. We could remove those default values, but then that would mean more work for people enabling forum v2.

Maybe tutor could set those defaults? Or detect when FORUM_MONGODB_CLIENT_PARAMETERS hasn't been changed? (The default parameters shouldn't be deployed without a password, right?)

Really though, it doesn't have to be the settings values. I'm just looking for any signifier of "this connection isn't functional, so we'll never find a course, so let's return None".

2. There are some cases when we need get_course_id_by_thread_id to not return None: that is, when forum v1 is enabled across the platform, but forum v2 is enabled only for select courses.

I realize this is likely my own ignorance (or lack of sleep), but I'm not clear why this is a problem. I agree that we need it to return actual course_ids at times, but if the forum app has a non-functioning connection to MongoDB, then it can't be running in a mixed v1/v2 mode to MongoDB anyway. As long as we're changing only the MongoDB backend's get_course_id_by_thread_id, it should still preserve behavior if the MySQL v2 backend returns that it has the course.

[regisb]

Maybe tutor could set those defaults?

Yes, you're right, and this reminds me that we forgot to include those default parameters in tutor-forum.

I'm just looking for any signifier of "this connection isn't functional, so we'll never find a course, so let's return None".

I understand. Yet, I'm reluctant to use this pattern, because it makes the codebase even more confusing than it already is.

I think we can avoid some of that complexity by introducing a global DISABLE_FORUM_V2 setting, which will be checked before course waffle flags. Wdyt?

I realize this is likely my own ignorance (or lack of sleep), but I'm not clear why this is a problem. I agree that we need it to return actual course_ids at times, but if the forum app has a non-functioning connection to MongoDB, then it can't be running in a mixed v1/v2 mode to MongoDB anyway. As long as we're changing only the MongoDB backend's get_course_id_by_thread_id, it should still preserve behavior if the MySQL v2 backend returns that it has the course.

Yes, you're right again. This will work, but the fact that it's difficult for us to wrap our brains around that solution tells us that maybe we should avoid it... I think it's much clearer if the app crashes because of incorrect settings than trying to guess what's the initial user intent.

[ormsbee]

I’m okay with a DISABLE_FORUM_V2 setting.

…

On Mon, Dec 9, 2024 at 6:30 AM Régis Behmo ***@***.***> wrote: Maybe tutor could set those defaults? Yes, you're right, and this reminds me that we forgot to include those default parameters in tutor-forum. I'm just looking for any signifier of "this connection isn't functional, so we'll never find a course, so let's return None". I understand. Yet, I'm reluctant to use this pattern, because it makes the codebase even more confusing than it already is. I think we can avoid some of that complexity by introducing a global DISABLE_FORUM_V2 setting, which will be checked before course waffle flags. Wdyt? I realize this is likely my own ignorance (or lack of sleep), but I'm not clear why this is a problem. I agree that we need it to return actual course_ids at times, but if the forum app has a non-functioning connection to MongoDB, then it can't be running in a mixed v1/v2 mode to MongoDB anyway. As long as we're changing only the MongoDB backend's get_course_id_by_thread_id, it should still preserve behavior if the MySQL v2 backend returns that it has the course. Yes, you're right again. This will *work*, but the fact that it's difficult for us to wrap our brains around that solution tells us that maybe we should avoid it... I think it's much clearer if the app crashes because of incorrect settings than trying to guess what's the initial user intent. — Reply to this email directly, view it on GitHub <#137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKFGKZKV2TAGK7QMOEX7D2EV5LPAVCNFSM6AAAAABTDVZXJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRXGY3DCNBVG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[regisb]

OK let's do this then. How do you want to proceed? Should we open a PR in the edx-platform master branch? Do we need it today?

EDIT: here's the Sumac PR openedx/edx-platform#35995

[ormsbee]

Please open a PR to the master branch that has the now-reverted changes that introduce v2 forums, and then add the commit from openedx/edx-platform#35995 on top of it. Please mention it in the Slack thread where this is being discussed.

[regisb]

OK here we go: openedx/edx-platform#36002