search

openedx / open-edx-proposals

Proposals for Open edX architecture, best practices and processes
http://open-edx-proposals.readthedocs.io/
Other
43 stars 32 forks source link

OEP-32 has been superseded and should be updated #407

Open e0d opened 1 year ago

e0d commented 1 year ago

The guidance for external ids in OEP-32 is currently incorrect and that OEP has been superseded by this ADR.

The OEP needs a significant rework.

robrap commented 1 year ago

Do you think the guidance is still correct for ids internal to the platform, and this is just about correcting guidance for external ids?

e0d commented 1 year ago

The ADR only covers the case of external IDs. So, job one would be correcting that.

My preference would be that auto increment database IDs were not used as identifiers, but that's a much bigger, breaking, change I believe.

robrap commented 1 year ago

I agree with your preference, but the point of the OEP was to codify what to use, given the reality of our situation. Better than using user id in some cases, and username in other cases, as was happening.

sarina commented 1 month ago

@robrap @e0d - is OEP-32 completely incorrect and should be marked as Obsolete, or is it simply that parts need updating?

robrap commented 1 month ago

It is partially correct. It is valid for internal ids, and invalid for external ids. I'm not sure if there are legacy exceptions to the external ids case that do use this same internal id.

sarina commented 1 month ago

Thanks. Assuming https://github.com/openedx/open-edx-proposals/pull/586 passes, I will mark this OEP as Needs Revision and point to this thread.

sarina commented 3 days ago

Put OEP-32 in Needs Revision status, please take a look at #603 - this ticket will be kept open for discussion on how to actually update the OEP for real