-
-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Resources] Add "beginner" (true/false) as a field to the /resources endpoint #157
Comments
Curious in that context, what prompted this discussion? What challenges would this attribute be aiming to solve? |
Why aren't we using the tags for something like this? |
@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. |
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? |
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. |
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 Rather than filtering down EDIT: There may also be a way to implement searching tags from 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. |
Good call! Updated the acceptance criteria with an example.
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.
++. 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. |
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.
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 Building on that -- if we did have a "beginner-friendly" tag in the system, it could be used in more contexts than just |
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.
Hmm... ok, good points. :) I've put a #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. |
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 |
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?)
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
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): #158 |
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.
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 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.
By "third status" I mean Will it be part of the search/filter fields?
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. |
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
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. |
🤔 ..and now that I think about it, Here is the tracking issue for the |
Awesome, thanks! |
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. |
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:
The text was updated successfully, but these errors were encountered: