thewca / worldcubeassociation.org

All of the code that runs on worldcubeassociation.org
https://www.worldcubeassociation.org/
GNU General Public License v3.0
330 stars 176 forks source link

Delegate Table Overhaul #4222

Open Jambrose777 opened 5 years ago

Jambrose777 commented 5 years ago

This is an epic that relates to issues #1931, #2185, #4185, and possibly more.

The goal is to split up the massive work it will take to refactor our usage of Delegates into their own tables.


Planned PR list:

  1. Create the tables needed for Delegates (listed below).
  2. Create a modify Delegates page (similar to edit teams) this will modify both the new Delegates tables and the old users tables. Remove the old way of editing Delegate status. (force use of the edit Delegates page)
  3. (not PR) add Delegate data. This will require research, probably with Ron and past Board to receive all the dates. I'll probably compile a spreadsheet to collect this data.
  4. Add Region Leads to the emails they should be included on, modify the Seniors that are cced (ie a Delegate Delegating in a country under a different Region).
  5. Modify the Delegates page to utilize the new tables, include subregions.
  6. Refactor remaining uses of Delegate columns in the users table to use thee new Delegates table.
  7. Remove the Delegate columns from the users table.
  8. Update delegates_must_be_delegates validation to correctly look at delegate history.

1, 2, and 3 will probably end up being just 1 PR.


Tables:

delegates
id
user_id
region_id (foreign key to delegate_region_countries) NOT NULL
subregion_id (foreign key to delegate_subregion_locations)
delegate_status
start_date
end_date
created_at
updated_at

restrictions:

delegate_regions
id
name
delegate_region_countries
id
delegate_region_id
country_id

Restrictions: all countries must be defined in this table.

delegate_subregions
id
delegate_region_id
name
delegate_subregion_locations
id
delegate_subregion_id
name
is_country

Some points about these tables:


Example: I'll use me.

delegate_regions:

id name
1 USA & Canada

delegate_region_countries:

id delegate_region_id country_id
1 1 USA
2 1 Canada

delegate_subregions:

id delegate_region_id name
1 southeast_usa Southeast USA

delegate_subregion_locations:

id delegate_subregion_id name is_country
1 1 Florida, USA false
2 1 Georgia, USA false
3 1 Tennessee, USA false

delegates:

id user_id region_id subregion_id delegate_status start_date end_date created_at updated_at
1 5569 1 2 'Regional' 2018-08-01 null 2019-06-16 2019-06-16
2 5569 1 2 'Full' 2017-09-01 2018-08-01 2019-06-16 2019-06-16
3 5569 1 2 'Candidate' 2016-07-01 2017-09-01 2019-06-16 2019-06-16

I'm open to suggestions on if the tables need to be composed in a different way while still keeping the information that we would want.

EdHollingdale commented 5 years ago

It will be good to have delegates reflected in the database how we think about delegates! But I assume completing this will be a lot of work. A few points:

Jambrose777 commented 5 years ago

Hey Ed,

  1. Phillippe's entries would look like this in the delegates table:
id user_id region_id subregion_id is_candidate is_full_delegate is_region_lead is_senior start_date end_date created_at updated_at
1 277 France null false true false false 2012-02-19 null 2019-06-16 2019-06-16
2 277 France null true false false false 2011-02-19 2012-02-19 2019-06-16 2019-06-16
3 277 Africa null false true false true 2018-09-01 null 2019-06-16 2019-06-16

This is using approximate dates and the region_id is substituted with the name for convience. Of note, I realized that He wouldn't be directly related to a country in Africa, so maybe delegate_region_countries also needs to have the continent listed (at least for Africa this would be true).

  1. This is an interesting case. The change would only need to be made in the delegate_region_countries / delegate_subregion_locations tables, but this doesnt preserve history (and is messy with the regions being combined / seperated). I don't really have a good solution yet to preserve this type of history.

  2. Maybe we leave phone numbers in the users table then!

pedrosino commented 5 years ago

One thing that I saw on first glance. It doesn't make sense to have 2 fields that are mutually exclusive. It works better to have a delegate_status column with either 'c' or 'f' meaning candidate and full, respectively.

From my old notes, I thought of recording promotion/demotion date. I see you plan on having multiple entries for the different delegate status. Not sure if that's the best idea, but feel free to explore.

About phone numbers, unless we start requiring/storing them for all users (doesn't sound like a great idea given GDPR), it's probably better to have it only for delegates.

pedrosino commented 5 years ago

Another thing: You propose is_senior and is_region_lead in the delegate table, but perhaps it would be better to have a separate table for this, something like

seniors id, delegate_id, region_id, start_date, end_date

region_leads id, delegate_id, region_id, start_date, end_date

Jambrose777 commented 5 years ago

Hey Pedro,

These tables mimic how we track dates and statuses in the teams tables. We don’t have separate tables per team / per position type, so I don’t think it’d make sense to do so here.

Jambrose777 commented 5 years ago

@EdHollingdale I think one solution to old regions combining would be to list out the old regions and the new regions. So for the US there would be 3 regions listed: USA East & Canada, USA West, USA & Canada. Then I would have another date change in my history for switch from the old region to the new region.

This still creates a little issue if a single country changed regions, so that would be a question if that has ever happened or not.

pedrosino commented 5 years ago

Hey Pedro,

These tables mimic how we track dates and statuses in the teams tables. We don’t have separate tables per team / per position type, so I don’t think it’d make sense to do so here.

I still think it would make sense to track seniors and leads separately, if only to know when their terms began/ended.

We could also think about storing this information for other teams.

Jambrose777 commented 5 years ago

We do store these dates for the other teams as well in the manner described above.

If you look at the example in my first post at the very bottom you can see the date for me becoming a region lead is stored.

pedrosino commented 5 years ago

Right, but what I'm saying is that maybe it's not the best option to have several rows in the delegates table for this.

One thing is the person being a delegate. That has a specific start and end date, and a status related to promotion. Other things are senior delegates, region leads or other roles we may have in the future. Separating those into more atomic tables/rows seems like a better plan regarding the (unknown) future. Searching for Philippe's information in this table would require a few extra filters, which may seem little now but can escalate over time.

Hopefully @jfly and @SAuroux can help

SAuroux commented 5 years ago

It doesn't make sense to have 2 fields that are mutually exclusive. It works better to have a delegate_status column with either 'c' or 'f' meaning candidate and full, respectively.

I fully agree with this.

But going by the same (or at least similar) argument, I think that it makes more sense to stick with the one delegates table rather than having multiple tables for multiple roles. I also don't understand what extra filters you would need, as far as I see it you just need to query for the user_id.

pedrosino commented 5 years ago

It's not the same case, I believe.

It's true we currently have a single table for team_members, but maybe we could change that as well. For example, in the WRT, there are 3 leaders listed: Tim Reynolds, Lars Vandenbergh and Sébastien. The first 2 have start and end dates, but we can't know for sure if they were leaders during that entire span. Same is true for Sébastien, who has a start date but no indication if he was always a leader or not.

My point is that one person can be a (candidate) delegate for a while, then become senior, then step down, a few years later become a region lead, step down, go back to senior, etc. They never stopped being a delegate, so it makes sense to have a single entry for them in the delegates table, while we stored the other roles separately.

With the model proposed, there would be several rows with repeated information in the delegates table, and also there's Philippe's case where there would be different regions listed for different periods and roles. By extra filters I meant the other columns (is_region_lead, is_senior and respective dates).

Jambrose777 commented 5 years ago

@pedrosino I think you miunderstand the tables currently in place. They DO track this information. Sébastien has a row for when he was just a team member. This has a start and end date. And a row for when he is the team leader, with just a start date. If he were to ever become a normal team member again his leader row will have an end date and another row is added without isleader being 1. So these periods of leadership ARE being tracked.

I don’t think there is any reason to change how teams tables work.

cubewhiz commented 5 years ago

It doesn't make sense to have 2 fields that are mutually exclusive. It works better to have a delegate_status column with either 'c' or 'f' meaning candidate and full, respectively.

I fully agree with this.

But going by the same (or at least similar) argument, I think that it makes more sense to stick with the one delegates table rather than having multiple tables for multiple roles. I also don't understand what extra filters you would need, as far as I see it you just need to query for the user_id.

Would it make sense, then, to extend this to all four types? Candidate, Full Delegate, Regional Delegate, and Senior Delegate?

Jambrose777 commented 5 years ago

It doesn't make sense to have 2 fields that are mutually exclusive. It works better to have a delegate_status column with either 'c' or 'f' meaning candidate and full, respectively.

I fully agree with this. But going by the same (or at least similar) argument, I think that it makes more sense to stick with the one delegates table rather than having multiple tables for multiple roles. I also don't understand what extra filters you would need, as far as I see it you just need to query for the user_id.

Would it make sense, then, to extend this to all four types? Candidate, Full Delegate, Regional Delegate, and Senior Delegate?

I'm not positive with that since you can be all of Full Delegate, Regional Delegate and Senior Delegate for a region at the same time.

viroulep commented 5 years ago

Yes but it means that this Delegate would have 3 rows in the table, each with an eventually different start/end time, and 3 different statuses (Full/Regional/Senior). All these statuses are completely independent, and most of the time the start/end time for all these position will be different.

So I don't see the benefit of having 4 different boolean columns when the use case "choose one and only one status among a list" is the exact definition of an enum.

And by the way I like the idea of having one single table and each row indicates a Delegate position held for a given period of time; it would allow us to be able to track history which we can't do now.

Jambrose777 commented 5 years ago

Hmm, so like:

Delegate | status | start_date | end_date Kit | Candidate Delegate | 2015-01-01 | 2016-01-01 Kit | Full Delegate | 2016-01-02 | null Kit | Senior Delegate | 2017-01-01 | null Kit | Region Lead | 2018-06-01 | 2019-01-01

I agree that that would make it cleaner to determine the Delegates individual roles and their history. So I'll fix that in the original post post-worlds :)

Jambrose777 commented 5 years ago

I've updated the original post from the individual fields, to 1 field for delegate status.

Jambrose777 commented 5 years ago

I’ve been thinking about this and it may make sense to split up the Delegates’ Locations into their own table. This way the Delegate’s duration in a role can be completely determined without having to look at when they’ve changed locations

Jambrose777 commented 5 years ago

Some updates after compiling some data:

jfly commented 5 years ago

(@Jambrose777, I think a google doc is a better place to do engineering design docs. IMO, github issues/email threads are too 1 dimensional.)

Would it make since to just add a column to the Countries table for this?

Do we think we'll ever want the history of "which countries were in which regions"?

Jambrose777 commented 5 years ago

(@Jambrose777, I think a google doc is a better place to do engineering design docs. IMO, github issues/email threads are too 1 dimensional.)

Would it make since to just add a column to the Countries table for this?

Do we think we'll ever want the history of "which countries were in which regions"?

Sure I can set up a google doc soon!

For the countries, yes/no. That will already happen with the Delegates table, but more importantly this would be used for ccing the correct Seniors when a competition is submitted. (Ie. If I delegate in South America, bot Kit and Nátan should be cced).

moralsh commented 4 years ago

@Jambrose777 what's the status of this?

I've wanted to redo the Delegates page for a while but I'd really like that it could reflect also regional Delegates, Can I help you with anything?

Jambrose777 commented 4 years ago

I've stopped work on it and do not plan to contribute further on the code implementation.

danieljames-dj commented 4 years ago

Actually I was thinking of taking this issue as I mentioned in a comment of another issue. @moralsh is it fine if we both do it together? Or do you want to take it up alone?

moralsh commented 4 years ago

I would very much love sharing this work @danieljames-dj :)

danieljames-dj commented 4 years ago

Great @moralsh . So, I'm giving my plan here (got some of the plan from the discussions at different threads):

  1. Basic software implementation for Delegates Table restructuring.
  2. Add the data to the new tables.
  3. Announce the new way of changing delegate status to the senior delegates and regional delegates.
  4. Enhancements in software implementation (which includes deleting delegate details from users table, adding regional delegates to email notifications, modifying delegates page to include regional delegates and subregions, etc).

I'll talk about 1st point here.

Basic software implementation for Delegates Table restructuring

We can divide it into the following sub-divisions:

  1. Create tables for Delegates, DelegateRegions, DelegateSubregions.
  2. Create Delegate Regions listing page, Delegate Region page and Delegate Edit page (pages visible only to people with permission).
  3. Remove the delegate edit details from user_edit page and have a button like ‘Edit Delegate Status’ which will redirect to ‘delegate_edit’ page.

I'll explain first two points:

1. Create tables for Delegates, DelegateRegions, DelegateSubregions

(Got the code from @Jambrose777's PR) The following is the structure of three tables:

1. Delegates Table

class CreateDelegatesTable < ActiveRecord::Migration[5.2]
  def change
    create_table :delegates do |t|
      t.references :user, null: false
      t.string :status, null: false
      t.references :delegate_region, null: false
      t.references :delegate_subregion
      t.references :country
      t.string :location
      t.date :start_date, null: false
      t.date :end_date
      t.datetime :created_at, null: false
      t.datetime :updated_at, null: false
    end
  end
end

2. DelegateRegions

class CreateDelegateRegionsTable < ActiveRecord::Migration[5.2]
  def change
    create_table :delegate_regions do |t|
      t.string :name, null: false
      t.string :friendly_id, null: false
      t.boolean :is_active, null: false
    end

    add_index :delegate_regions, :friendly_id

    # Will add insert migrations here
  end
end

3. DelegateSubregions

class CreateDelegateSubregionsTable < ActiveRecord::Migration[5.2]
  def change
    create_table :delegate_subregions do |t|
      t.string :name, null: false
      t.references :delegate_region, null: false
      t.string :friendly_id, null: false
    end

    add_index :delegate_subregions, :friendly_id

    # Will add insert migrations here
  end
end

I'll copy-paste the insert migrations from this spreadsheet.

2. Create Delegate Regions listing page, Delegate Region page and Delegate Edit page

Delegate Regions listing page (worldcubeassociation.org/delegate_regions)

This page will have a table that will list all the active delegate regions. Each row will have two columns - Region name and the Senior Delegate. On clicking a row, it will open the respective Delegate Region page.

Delegate Region page (worldcubeassociation.org/delegate_region/)

This page will list all the active delegates of the region in the following order:

For the people with permission, each row will have an edit button that will take you to the Delegate Edit page.

Delegate Edit page (worldcubeassociation.org/users//delegate_edit)

It will be a form having the following details:

  1. Delegate status - dropdown with empty/trainee/junior/full
  2. Delegate region - dropdown will have all the active delegate regions
  3. Delegate subregions - dropdown will have all the active delegate subregions of the selected region (can be empty also)
  4. Three Buttons: 4.1. Update - Will be active if any of 1-3 is changed 4.2. Appoint as Regional Delegate - Visible only if the user is not a regional delegate, but is a delegate. When this is clicked, it will unhide 5-7 which will be hidden 4.3. Appoint as Senior Delegate - Visible only if the user is not a senior delegate, but is a delegate. When this is clicked, it will unhide 8-10 which will be hidden
  5. Same as 2, but for regional delegate
  6. Same as 3, but for regional delegate
  7. Two Buttons: 7.1. Update - Will be active if any of 5-6 is changed 7.2. Remove Regional Delegate - This will remove the regional delegate status of the delegate
  8. Same as 5, but for senior delegate
  9. Same as 6, but for senior delegate
  10. Same as 7, but for senior delegate

Please comment your suggestions regarding this. We can start the work as soon as possible. :-)

moralsh commented 4 years ago

Great! I like the way you have divided the work, and I don't have anything to add right now on points 2-3, however, I think we may have to think the model a bit more. I'll try to come up with a proposal during this week. I might start a doc as was proposed earlier in the thread.

I also think this PR should cover only model part and should move the rest to different PRs, and also, as a last step I'd add a Delegates public page rework which may be interesting to do having the work on website redesign in mind.

moralsh commented 4 years ago

After giving some thought to all of this the last few days I'd say the Delegates table might be okay as it is now, it'll probably need some indexes but we could define them once we're sure how are we querying the table. I understand that the status field could be trainee, junior, full, subregion lead and region lead, Am I right?

About DelegateRegions and DelegateSubregions I think they should lose the Delegate in the name and simply be called Regions and Subregions. I also think we should add a top level (maybe Continent) and just reference it in the Regions table. There's no other use right now for this table, but dues and regional organization might use it in the future.

viroulep commented 4 years ago

@danieljames-dj or @moralsh, could one of you please create a draft PR with the suggested db structure (I'm happy to read migration files, but I believe using the actual column names would be easier/more explicit as how information is stored/referenced)?

The draft PR would never be merged and can just be a text file: the point would be to have a mean to comment on specific points of the schema (eg: table/column naming), without "polluting" this rather "high-level" issue.

More generic question: do we have a source about how to seed our regions and subregions tables? I mean I actually can't tell what is a valid region or subregion! Is that something which change often? Should we have a UI to change them?

I understand that the status field could be trainee, junior, full, subregion lead and region lead, Am I right?

We should probably stick with the WCA names, so: trainee, junior, delegate, senior, regional. It's worth noting that currently the "Junior Delegate" status is still represented by the "candidate_delegate" status; this is hardcoded in a bunch of places of the codebase, so migrating that is probably not as simple as changing the status value!

Jambrose777 commented 4 years ago

I also think we should add a top level (maybe Continent) and just reference it in the Regions table.

Not every region belongs to 1 continent, so I would not recommend this.

do we have a source about how to seed our regions and subregions tables?

Yes (and I DMed you a spreadsheet)! And Region structures have changed 4-5 times over the WCA's lifespan, so I'm not sure whether adding a UI to do so is worth it.

We should probably stick with the WCA names, so: trainee, junior, delegate, senior, regional.

And temporary.

viroulep commented 4 years ago

And temporary.

Right!

moralsh commented 4 years ago

Not every region belongs to 1 continent, so I would not recommend this

I'm not gonna argue a lot in favor of creating the Continents table as it really isn't that important to have it, but there are several ways to circumvent the problem of central Eurasia, I had thought of it as a way to just giving context to regions and adding a way to hierarchically show regions. Anyway ordering regions by region name also distributes them into continents, so that could be more than enough for now.

I'll write down the tables as they are now and will create the draft PR later or tomorrow depending on my day.

saranshgrover commented 3 years ago

Hi - please note that the Board recently encountered a situation where we are Seniors for 2 regions. 1 account is not able to be a senior for multiple regions, and we would like that option to be available. Please keep this in mind with the delegate table overhaul - let me know if this should be its own issue.

Jambrose777 commented 3 years ago

Hi - please note that the Board recently encountered a situation where we are Seniors for 2 regions. 1 account is not able to be a senior for multiple regions, and we would like that option to be available. Please keep this in mind with the delegate table overhaul - let me know if this should be its own issue.

With the table structure that is set up in the draft PRs, this would be possible, so I wouldn’t expect any issue with it!