Adding some checklist bullets in an updated PR template?

[lpatmo]

Proposal: update the PR template to look something like what frontend has:

Second proposal:
Add some checklists that are Django-project specific.

For example, things like:
[Y/N] Did you update the model?
[Y/N] If so, did you remember to run [migration commands]
[Y / N / NA] If you changed an existing model, did you provide a default values for the new field, and retroactively add values for the older data? (We strive to not have null values)

^ So... that comment about striving not to have null values is an assumption I made re: code standards that we should maybe discuss a little more and log as a decision if it ends up being that.

But the general idea I'm wondering about / proposing is if we can update our PR template -- and contributing docs, of course -- to have some checklists that can help folks get past gotchas in the code.

[BethanyG]

I think those are all good things for a checklist, but I think it is WAY too late to be thinking about them when submitting the PR.

I think those prompts belong either in a general checklist for documentation, or as Acceptance Criteria for any issue (issue templates for these variations, perhaps??) that includes changes to models. Things like:

• default values
• whether or not the field is nullable,
• whether or not the field will be required

are for specification - we should have those in place and documented as part of the work spec, and not have any expectation that a contributor will know to ask for them, or know what reasonable defaults are for them.

Also? The code will fail (and in most cases Django will toss warnings or won't even start) if someone hasn't made migrations after adding to a model and tries to write code that uses the new field...so they really need to do that themselves on their dev setup, or they won't be able to make dependent changes to things like serializers or views ... or any tests.

Now - they may do all that and forget to commit a migration or data migration file..so maybe we can add Did you check in your migration files?.

Although for migrations, we have a GitHub action in place that will checkout the code, run makemigrations, and if it gets a migration back (meaning the submitter didn't make or check one in), then the PR fails - so that will also poke someone. So we owe @chris48s ⭐ 🌟 ⭐ for that!

[lpatmo]

or as Acceptance Criteria for any issue (issue templates for these variations, perhaps??) that includes changes to models.

💯 Yeah, I think there's no harm in double checking in the PR template, but making sure some common steps are in acceptance criteria/issue templates are a better idea 😂 I'll have to think a bit on what exactly that template should look like, though, because not all acceptance criteria will necessitate model changes and the associated checklist.

Most cases one of the core maintainers will be writing the issue based on specific product specs (after the user research/designs have progressed), but I can see newer contributors creating issues too, and for those I'll have to think a bit more on the type of issue they'll most likely create, and what needs to be in the template.

[BethanyG]

I think it is possible to have more than one issue template, and to then have a list users can pick from. I stole the screencap below from the Exercism V3 reopo. They made these different issue templates to try and help contributors file the "right" type of issue (I think different issue types may ping different core members, and each has different explanations, links, and categories).

[lpatmo]

Ohh, yes! When the Discussions tab got turned on for us I remember it being suggested that edit our issue template to have "Ask a question" point directly to a new GitHub discussion.

I think different issue types may ping different core members, and each has different explanations, links, and categories

That's very cool. I'm guessing that's via Github Actions?

[BethanyG]

Propose:

What type of PR is this? (check all applicable)

• 🐛 Bug Fix
• 🎨 Enhancement
• ♻️ Refactor
• 📝 Documentation
• 🔖 Release

Context

Placeholder example below:

Closes Issue #128

Right now, on the front-end, we need to pass in a token (i.e. the user needs to be logged in) before they can see a list of resource.

 axios
      .get('/api/v1/resources', {
        headers: {
          Authorization: `Bearer ${authContext.authTokens.token}`,
        },
      })

This should be true for when users are creating a resource, but the list of resources on https://cb-react-concept.netlify.com/resources should be available to the public.

Other Related Tickets & Documents (as needed)

Placeholder example: This relates to issue #23, and I also filed issue #43 as a next step to do after this PR is merged.

Implementation Details

What was your thought process as you changed the code? What does someone need to consider in reviewing it?

Placeholder example:

• Made GET /resources not protected by authorization
• Changed setup for tests to make sure we're not authed for the GET requests, but are authed for POST, PATCH, and search.
• Added test to make sure GET /api/v1/resources/{{guid}}/ and GET /api/v1/resources/ requests and search work without a token
• Altered tests for GET GET /api/v1/resources/{{guid}}/ and GET /api/v1/resources/ requests and search to ensure that they also work with a token
• Added tests to make sure PATCH and POST fail without a token
• Added test to make sure DEL fails without a token

Any New Libraries Added??

• I've updated requrements.txt

List the new libraries here:

Any New Migration Files Added??

• 👍 yes
• 🙅 no, because they aren't needed

Did You Add Tests?

code added or changed without test coverage or good reason for exemption won't be approved.

• 👍 yes
• 🙋 no, because I need help
• 🙅 no, because they aren't needed

Did You Add Documentation?

• 📜 readme.md
• 📜 contributing.md
• 📜 wiki entry
• 🙋 I'd like help writing documentation, and filed and issue for it
• 🙅 no documentation needed

[lpatmo]

😍 That looks great!

I'd also add something like:

Does this PR create or update an API endpoint?

[ ] Yes, and this is the expected request/response:

Example:
GET /resources

Response:

{
    "count": 47,
    "next": "http://localhost:8000/api/v1/resources/?page=2",
    "previous": null,
    "results": [
        {
            "guid": "4e85dbb6-d519-11ea-a622-0242ac130002",
            "author": "",
            "title": "asldfkjadsf",
            "description": "",
            "url": "https://gitduck.com",
            "referring_url": "",
            "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": []
        },
    ]
}

[BethanyG]

Again - this is something for specification and would be part of the description or the issues linked in context or related docs. Forcing someone to re-confirm or cut and paste (with the possibility of mistakes) both confuses a new contributor, and is irrelevant to the person reviewing the PR.

As a contributor, I have no idea what to do with this, unless I have the context of an issue or specification that already has it. Even worse - if I am working on a a piece of the API that doesn't return JSON, I might think I need to paste something - anything - in here. And in doing so I duplicate or munge information.

As a reviewer, I want to see what someone was working from as a spec, and where they deviated. (which should be noted in context or implementation details).

I don't need a re-confirmation of something that should be agreed upon before or during the coding process. And if it is changing during the process, I would expect to hash it out in the issue discussion or the specs. But mostly in a linked issue discussion.

I also think that having really long PR templates with lots of checkboxes will scare off contributors. It's another wall they have to navigate, and more questions they have to stop and get answered. I'm tempted to remove the first section to shorten things.

If we want to backdoor prompt users to document, we could suggest that under documentation. Something like if you've added or change an API endpoint, it might be a good idea to document that for others on the project

[lpatmo]

Yes of course, the API request/response request requirements should already be defined in the issue itself! Sorry I was a little scatterbrained last night.

By "first section" did you mean "What type of PR is this? (check all applicable)" or removing the "context" section? I'll create a PR right now for the new PR template, and get additional feedback there.

... and then maybe open an issue to update the issue template with some tips about how to write good acceptance criteria. :)

[lpatmo]

Put up a PR here: #159

Happy to make changes there!

[lpatmo]

(#159 was merged)