codebuddies / backend

CodeBuddies back-end
https://codebuddies.org
GNU General Public License v3.0
20 stars 25 forks source link

[Resources] Add "beginner" (true/false) as a field to the /resources endpoint #157

Closed lpatmo closed 3 years ago

lpatmo commented 4 years ago

Context

In the design hangout with @adachiu a few weeks ago, we raised the idea of adding a beginner boolean to /resources as a new field (instead of specifying beginner/intermediate/advanced).

Acceptance Criteria

[ ] Beginner (true/false) should be returned as a field when querying a resource [ ] Add beginner to the model and make sure migration files are checked in [ ] Serialize [ ] Write tests

Example JSON response from a GET /resources call:

 {
            "guid": "4e85dbb6-d519-11ea-a622-0242ac130002",
            "author": "",
            "title": "asldfkjadsf",
            "description": "",
            "url": "https://gitduck.com",
            "referring_url": "",
            "beginner_friendly": true,
            "other_referring_source": "",
            "user": {
                "id": 229,
                "username": "aug2nd",
                "first_name": "Linda",
                "last_name": "Peng",
                "is_superuser": false
            },
            "date_published": "2020-08-02T16:38:45.651676-07:00",
            "created": "2020-08-02T16:38:45.652112-07:00",
            "modified": "2020-08-02T16:38:45.651710-07:00",
            "media_type": "Podcast",
            "paid": true,
            "tags": []
        },
angelocordon commented 4 years ago

Curious in that context, what prompted this discussion? What challenges would this attribute be aiming to solve?

lpatmo commented 4 years ago

IIRC, the historical discussion happened something like this:

IMO, implementing this particular PR isn't super high priority, but will be a good refresher for how to change the API and add/remove fields in future cases.

Anyway, hope that explanation (mostly from my memory) helped explain the context! :)

angelocordon commented 4 years ago

Why aren't we using the tags for something like this?

BethanyG commented 4 years ago

@angelocordon -- tags aren't easily searched or filterable at this point. They are also at risk of becoming quite messy to maintain.

@lpatmo We need to remove "good first issue" from this. This is not a beginner friendly issue, as it requires changing an existing model, view, and serializers -- and the addition of multiple tests -- and may have other considerations as well.

There also needs to be discussion around the criteria of what makes a Resource "beginner friendly" -- and scope that for the UI. IMHO, if we are not clear about what we consider beginner friendly and surface the criteria, this will quickly become a meaningless field. Will this flag be a required field for creating a resource? If not, there will be either nulls in the DB or a third status to deal with. Will it be part of the search/filter fields?

Additionally, it would be good to specify the JSON that needs to be returned from the endpoint.

BethanyG commented 4 years ago

Also -- on reviewing the Slack discussion, thinking through the feedback mechanism (if any) would be good. Would community members up or down vote or "report" a resource as not being beginner friendly? And would we use this tag to surface resources -- or gather them into collections for display?

angelocordon commented 4 years ago

Hmm, I think I might have a misconception of what resource tags are for. If we can't search for resources with a JavaScript or Python tag, how do we surface up particular resources? Are there other keys that we use for that?

I suppose despite the challenges of not being able to query or filter them, how are we thinking about the difference between a beginner label vs a normal resource tag?

This side of the project is certainly not my domain so feel free to correct me here so that I can have a better understanding of how this would tie to the frontend eventually.

BethanyG commented 4 years ago

No -- I think you are asking good questions, Angelo. Here is a discussion thread with more tag related questions, with links to background info and docs. TL;DR: theres more discussion to be had, and more thinking to do around how/why/when/what we tag -- and what our intentions/uses are for it. I'd welcome your thoughts and suggestions on that.

Part of the issue with tags is that they are not part of the Resource model. While associated tags are returned as a convenience for a Resource, Tags are their own model, and are a many-to-many relation with Resources. So - it's not that we can't search and filter ... just that doing so requires a different "path" than searching against the Resources endpoint. And we haven't thought through implementation details around that.

Rather than filtering down Resources based on Tags, we'll have to query a Tags endpoint (I think) to then find the associated Resource guids, and then retrieve those guids from Resources -- so that would need to be speced and scoped. So adding a field directly to Resources avoids that conversation/spec (not that that makes it better - just quicker).

EDIT: There may also be a way to implement searching tags from Resources - but I would have to dig for it. I think we decided at the time not to do it, and went in favor of having a separate Tags app/endpoint. But we could research/scope it, if it felt better to do so. IIRC...since we thought tagging would apply to much more than Resources, we felt Tags would be better as their own thing, and not implemented in every other endpoint....but it is a bit hazy now.

But more importantly, we risk creating a mass of tags that can quickly become a hard to maintain bottleneck in the DB -- especially since we've gone back and forth on weather we'll allow users to create their own tags on entry. De-duping and "cleaning" could be a problem.

There is also a context problem, since tagging can be applied to many things -- but not all tags will be meaningful in all contexts. I'm also thinking about the mess that you might see with hashtags on Twitter or Instagram...and worrying that we might not be able to build a system or interface that will let us administer this easily.

lpatmo commented 4 years ago

Additionally, it would be good to specify the JSON that needs to be returned from the endpoint.

Good call! Updated the acceptance criteria with an example.

Would community members up or down vote or "report" a resource as not being beginner friendly? And would we use this tag to surface resources -- or gather them into collections for display?

I can imagine users wanting to filter by this tag. The idea of voting/reporting is interesting, but I wouldn't prioritize it. I mainly want to document the steps (via the PR corresponding to this issue) about how to add a field to the API, as a guide for future new contributors.

IMHO, if we are not clear about what we consider beginner friendly and surface the criteria, this will quickly become a meaningless field.

++. Let's show this to the user on the front-end on the helper text for the checkbox. I started a discussion here re: what we want to put in the helper text.

BethanyG commented 4 years ago

how to add a field to the API, as a guide for future new contributors.

This is much better left for a PR or set of documentation on adding a new endpoint. IMHO. Not changing an existing endpoint. When adding a new endpoint, beginners can refer to one that is already working to see what to add and do - and we can provide them template code (and clear and detailed specification, I would hope). There is also pretty good documentation out there for them to read and follow. Also much less risk of them breaking existing functionality, since their changes would be in a new, added feature.

Will this flag be a required field for creating a resource? If not, there will be either nulls in the DB or a third status to deal with. Will it be part of the search/filter fields?

We need that info for the model. Also a default value (which I assume will be "False"). Also keep in mind that if there is any existing data in your DB, it will need to be changed or dropped before/during migration if this field is added. We'll also want to specify if this is now a a field for search.

But I also want to explore what @angelocordon has brought up above. Why not use Tags for this? You've even called this a tag....and this is ostensibly what tags get used for in other systems. Complicated or not, we should think through how we are using them, and how to best filter and search on them.

Building on that -- if we did have a "beginner-friendly" tag in the system, it could be used in more contexts than just Resources - it could be used to tag hangouts, projects, or other things...and that would solve the problem of users weighing in -- because they could decide to apply the tag or not....so maybe we have some more discussion around what we intend? It feels like we've not really resolved that - and getting clearer on that can help us decide if we need this flag or not.

lpatmo commented 4 years ago

This is much better left for a PR or set of documentation on adding a new endpoint. IMHO. Not changing an existing endpoint. I think both are important, TBH. Having a guide for adding a brand new endpoint is important, but so is changing an existing one so that additional features can be added or tweaked after user feedback. (Am thinking of a user feedback -> design tweak cycle). Ideally we get most of the API right on the first try, of course, but I don't think it can be guaranteed.

Will this flag be a required field for creating a resource? If not, there will be either nulls in the DB or a third status to deal with. I was thinking it won't be a required field. What do you mean by a third status to deal with?

Will it be part of the search/filter fields? "Filter" would be nice but not priority, and I don't think it needs to be a part of search.

Also a default value (which I assume will be "False"). Yep, I agree the default should be false.

Also keep in mind that if there is any existing data in your DB, it will need to be changed or dropped before/during migration if this field is added. Oh god, I forgot about data migrations 🤯 (I think in past experiences, I've just made all the new field values be... null)

Yeah, would love to figure out how to -- after adding a new field to an existing endpoint -- give the old data a default value for that new field. That would save us when we have production data.

But I also want to explore what @angelocordon has brought up above. Why not use Tags for this? You've even called this a tag....and this is ostensibly what tags get used for in other systems. Complicated or not, we should think through how we are using them, and how to best filter and search on them.

Building on that -- if we did have a "beginner-friendly" tag in the system, it could be used in more contexts than just Resources - it could be used to tag hangouts, projects, or other things...and that would solve the problem of users weighing in -- because they could decide to apply the tag or not....so maybe we have some more discussion around what we intend? It feels like we've not really resolved that - and getting clearer on that can help us decide if we need this flag or not.

Hmm... ok, good points. :) I've put a hold on this issue. I still think it will be useful to document how to change an endpoint without breaking things / causing chaos for the frontend, so will think about another issue where we can practice that.

https://github.com/codebuddies/backend/discussions/154 is the best place for the tagging discussion, right?

I can see people clamoring for the ability to add tags to "I am looking for____ " posts, so agree, it's tempting to figure out.

On the other hand, we can still launch the "I am looking for___" and resources list without tags, and only rely on basic search and filters.

BethanyG commented 4 years ago

Yeah, would love to figure out how to -- after adding a new field to an existing endpoint -- give the old data a default value for that new field. That would save us when we have production data.

So there is this info on Data Migrations - though I've not written one myself.

When I've done this in the past, I've defined a few things -- I make the field required and not-nullable, and I set a default value in the model. I've then been stopped during migrations and asked to either verify the default or fill in the value for records already in the DB....and it feels like it never really goes well....so perhaps it's time to get jiggy with writing a formal Data Migration.

We should also keep in mind that the more endpoints we build and the more data we add, the more considerations like this we have to think through...and that is not a bad thing -- but it is work we need to add to our checklist.

I always go on the philosophy that nulls should be avoided whenever possible - they're just a PITA to account for in joins and groupings, so I try hard not to null anything unless it really needs to be null...but I'm also lazy. 😁 .

lpatmo commented 4 years ago

I've then been stopped during migrations and asked to either verify the default or fill in the value for records already in the DB...

Oh man, it really has been a long time, because now I vaguely remember that Django migrations did ask me in the past to fill in values for those new fields, including asking whether I wanted to have them be null or not. (maybe?)

When I've done this in the past, I've defined a few things -- I make the field required and not-nullable, and I set a default value in the model.

Did you mean you generally try to make those fields required and not-nullable as a practice? Or mainly non-nullable, right? (As in, the field can either be required or not, but you strive to avoid nulls)

We should also keep in mind that the more endpoints we build and the more data we add, the more considerations like this we have to think through...and that is not a bad thing -- but it is work we need to add to our checklist.

YES... and speaking of checklists, I'm reminded how on github.com/codebuddies/frontend, we recently revamped the PR template to include checks like "did you write tests?" and "did you add documentation?"

This is totally off-topic from this issue, so I'll move this to a discussion post, but your comment just made me think about how it might be a good idea to add some backend-specific checklists into an updated PR template for the backend. Will share an example in that discussion post (here): https://github.com/codebuddies/backend/discussions/158

BethanyG commented 4 years ago

I think both are important, TBH. Having a guide for adding a brand new endpoint is important, but so is changing an existing one so that additional features can be added or tweaked after user feedback. (Am thinking of a user feedback -> design tweak cycle). Ideally we get most of the API right on the first try, of course, but I don't think it can be guaranteed.

Totally agree - but having those tweaks be done by someone new to the project - or to Django/DRF - who may not know any of the context sounds like a fairly high risk endeavor - for both the contributor and the project. I think there are better ways to get someone involved, and get them making meaningful changes on the back end. We don't want to set someone up for breaking back end code, front end code, and any other processes that rely on the existing API. Yes - that's true for any of us..but there is a higher chance of that happening with a brand-new contributor.

I still think it will be useful to document how to change an endpoint without breaking things / causing chaos for the frontend, so will think about another issue where we can practice that.

Agree that someone should document anything unique we do in adding/changing an endpoint. We probably should log a documentation issue about that. But we should also be writing issues/feature requests that include relevant links to documentation that we expect contributors to read and understand. I don't think we should be re-writing or re-summarizing Djago, Postgres, or DRF documentation.

Maybe creating a checklist of files or tasks to complete/making a video of the steps/streaming the steps/creating an interactive tutorial via a special branch with progressive commits -- rather than trying to dream up a "practice" task.

Or - if you really feel a "practice task" is something needed, maybe we resurrect the projects endpoint, and kill two birds with one stone by creating a learning project for adding that endpoint and iterate on it? I spent a long time documenting those related issues and linking in documentation, and thought I did a fairly good job of it - so maybe someone can go through that and add additional notes on how they'd make changes once things were implemented. And we could consolidate the multiple issues into one, if that felt better.


Will this flag be a required field for creating a resource? If not, there will be either nulls in the DB or a third status to deal with.

I was thinking it won't be a required field. What do you mean by a third status to deal with?

By "third status" I mean null...which.....just feels muddy. A Resource either meets the criteria for "beginner friendly", or it doesn't....there really should be no in-between. But setting a default will probably fix that, and then we may not need to have it be formally "required". And setting a default and then marking the field in the model as not nullable should cover us.

Will it be part of the search/filter fields?

"Filter" would be nice but not priority, and I don't think it needs to be a part of search.

What would be the purpose for this field if you can't search or filter it? Would it simply be for display (which is fine) -- I just haven't gone over the FE specs closely, so I don't have a clear mental image of what the use case is for this.

lpatmo commented 4 years ago

Or - if you really feel a "practice task" is something needed, maybe we resurrect the projects endpoint, and kill two birds with one stone by creating a learning project for adding that endpoint and iterate on it? I spent a long time documenting those related issues and linking in documentation, and thought I did a fairly good job of it - so maybe someone can go through that and add additional notes on how they'd make changes once things were implemented. And we could consolidate the multiple issues into one, if that felt better.

That sounds excellent. :D I wasn't feeling fully comfortable writing any documentation about it because I haven't gone through the experience of creating or an updating an endpoint myself, so I wanted to work on something small that confirmed some pre-conceived notions I had about the steps required for updating an endpoint. Using projects as a sandbox sounds good.

What would be the purpose for this field if you can't search or filter it? Would it simply be for display (which is fine) -- I just haven't gone over the FE specs closely, so I don't have a clear mental image of what the use case is for this.

Mainly for display, yeah. I meant being able to filter by 'beginner' would be nice but not a priority if it's complicated to sort out, since the priority is the hangout flow.

BethanyG commented 4 years ago

🤔 ..and now that I think about it, projects is another place we might have a beginner-friendly flag or tag...so that might also be a "sandbox" we use for trying out different ways of conveying/playing with that.

Here is the tracking issue for the projects endpoint. It has all the other "smaller" stuff linked. Too bad we can't glue it all together..but maybe when I get time I can. https://github.com/codebuddies/backend/issues/79.

lpatmo commented 4 years ago

Awesome, thanks!

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.