usgo / agagd

American Go Association Games Database (AGAGD)
https://agagd.usgo.org
MIT License
47 stars 30 forks source link

Chapter code deprecation #107

Closed annabunches closed 4 years ago

annabunches commented 4 years ago

Changes all references to chapter codes to prefer the numerical chapter ID from the Chapters table (for internal references) or the chapter name (for display purposes). Chapter codes still work in URLs for legacy purposes (i.e. to avoid breaking bookmarks) and will print instead of the chapter name if the chapter doesn't have a populated name field.

I was unable to test these changes in dev, since I can't get fixtures to load. The django models seem to be sufficiently out of phase with the DB tables that trying to write via the django app fails in several unpredictable ways. We'll want to do substantial testing on the dev site as a result.

Fixes #93.

michaelhiiva commented 4 years ago

Just considering there is also another fall back in the tables that displays a dash which we might want to change to have a fall back with the chapter codes as well.

https://github.com/usgo/agagd/blob/f15012cb3a16ff6422a1209718639975f1bc0139/agagd/agagd_core/tables.py#L100-L109

annabunches commented 4 years ago

re: the tables code, this PR includes changes to one of those methods, looked like I missed one though.

michaelhiiva commented 4 years ago

RE: Chapter Code URLs

I think we should not be keeping the chapter code urls as is but redirecting them to a chapter id url if we are to keep legacy support for the chapter code urls because we are already moving to eliminate them from titles and other areas. I think URLs should reflect this for consistency.

annabunches commented 4 years ago

That's a really good point. I'll figure out how to do that and update the PR :)

annabunches commented 4 years ago

This is now back in a state where I believe it to be done. However, we may want to wait until after merging the fixtures code, so we can do local testing before pushing to the dev server. (I've only been able to run a basic smoke test locally - this code runs just fine with a nice, empty database)

annabunches commented 4 years ago

OH! There is also another important blocker before merging this: some users appear to have chapter and chapter_id values that do not point to the same club! We need to figure out how many of those cases we have and how to deal with them.

vash3g commented 4 years ago

'chapter' is the chapter code and 'chapter_id' is the chapter agaID. This is not properly updated in the membership code to update that column as well.

annabunches commented 4 years ago

@vash3g yep, the problem is that several users are now listed in one chapter but in that very chapter's view, are shown as belonging to a different club, and we need to be confident that we know which club they "meant" to be in. (because switching to using all chapter_ids will represent a change compared to the data that is currently shown in prod)

Update: after perusing the membership repo, it looks like all the code there now updates the chapter_id, never the chapter. So it seems very probable that the chapter codes displayed in the chapter members table (for an example, see https://agagd.usgo.org/chapter/NOVA/) are invalid where they appear different than the current chapter. (Note on that page, for instance, the chapter listed for AGA members 8051, 5109, and 8822) In other words, the chapter_id field should always be correct.

annabunches commented 4 years ago

The latest changes have now been tested with the new fake fixtures, and appear to work. Please take a look!

annabunches commented 4 years ago

Ah, discovered that I didn't test the chapter code redirects, and they're currently broken. Working on it :)

annabunches commented 4 years ago

There we go, now fixed :)

michaelhiiva commented 4 years ago

This should fix the issue of loading /chapter/[id] in pull #129.