ckan / ckanext-issues

CKAN Issues Extension
https://github.com/ckan/ideas-and-roadmap/issues/4
MIT License
14 stars 31 forks source link

Issue number should start at 1 for every dataset #35

Closed davidread closed 9 years ago

davidread commented 9 years ago

It's weird when you go to a dataset and the issues are numbered 3, 46 & 105. Even accounting for some closed issues, it suggests there have been over 100 issues in total for the dataset. It rubs with the commonly assumed mental model of a dataset having issues starting at 1.

rufuspollock commented 9 years ago

@davidread I agree on this. My one worry is just the cost of implementing this. @joetsoi what is your estimate for the refactor to make this work. I guess we keep the primary key as is as auto-increment but add a new field called "issuenum" or something and have logic to set that when the issue gets created by looking up the highest number of existing issues and then assigning that + 1.

(one problem there is deleted issues but i guess we live with fact that you can reassign an old issue number - the only alternative would be having issue "states" but i think that adds more complexity we don't need)

joetsoi commented 9 years ago

keeping the primary key and adding a new field is what I had in mind. I'd estimate between probably 1-2d (probably erring on 2 to catch everything), as we've having to change all aspects model/action/controllers and it affects all actions and all controllers.

Another possibility is just not allow deletion of issues at at all (it's what github does). It feels slightly awkward if a link is not just broken, but could possibly change to be pointing at a different issue than what it did originally. Eitherway, adding a state/not allowing deletion/accepting links can change shouldn't affect the code changes needed here to fix the numbering.

rufuspollock commented 9 years ago

@joetsoi makes sense and 2d seems reasonable esp with tweaks to / adding of tests. My one question is how disruptive we have to make it. Internally we can still look up by unique id - you just need a quick function to map (dataset, issunum) => issueid ... (?). Anyway this is your area of expertise.

Aside: github does allow you to delete issues (you just need to be admin on the repo i think).

My one question to @davidread is how important we think this feature is compared to other items.

joetsoi commented 9 years ago

Well the changes to the model are necessary to add the additional field.

I did consider just making the changes to the controllers so the url links and the frontend. But then you'd end up with some double validation going on and whoever uses the api would have to use issue_id. So we could just leave the action functions as is, but the api would be difficult to use and the controller code a bit ugly. For the sake of any api user or future maintainer i thought it'd be best to fix the action functions as well.

If you really wanted no disruption at all, I would consider changing the primary key to be a unicode string, you'd end up with urls like //issue/99dbfe82-5898-4472-ac83-7a04a0bc79c9 and we can just display the issue number on issues listing. But you'd remove some of the utilty of numbering in the first place if we did this.

rufuspollock commented 9 years ago

@joetsoi definitely agree we need this in the actual model database. More about the tweak in controllers etc. The unicode string is def not a good idea - i agree.

Up to you on the implementation (re validation i'm not sure i quite understand though: basically the issue_num should either not be set - on create - or should never be changed). Anyway i leave this to you to make the call on implementation :-)

davidread commented 9 years ago

We've decided this is essential for our launch. Great to see there's a viable option to implement it. Cheers.