Skip to content
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

Improve issue templates & add PR template #7051

Merged

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Dec 8, 2020

Improvements feature suggestion template:

  • Adds basic checkboxes as in the bug report template:
    • Prevents disclosure of security vulnerability in context of a feature suggestion.
    • Ensures that a basic concept of the feature exists, or encourages to discuss the feature previously in the Parse Community Forum.
  • Rephrases the **Is your feature request related to a problem? Please describe.** which is almost every time answered with No, likely being confused as being a bug report instead of a use case description.
  • Adds use-case, which can make it easier to understand the feature at a glance.
  • Adds a 3rd party reference, something which is often part of a feature evaluation, but we could shift some of that research effort to the issue author.

Improvements bug report template:

  • Adds PR / test case section
  • Adds link to contribution guide

If the templates work well, we can adopt them for other repos. This is in line with the new labels that are being tested in the Parse Server repo first, then rolled out to other repos.

* commit 'ccb045b68c5b4d983a90fa125513fc476e4e2387':
  fix: upgrade @graphql-tools/links from 6.2.4 to 6.2.5 (parse-community#7007)
  fix: upgrade pg-promise from 10.7.0 to 10.7.1 (parse-community#7009)
  fix: upgrade jwks-rsa from 1.10.1 to 1.11.0 (parse-community#7008)
  fix: upgrade graphql from 15.3.0 to 15.4.0 (parse-community#7011)
  update stale bot (parse-community#6998)
  fix(beforeSave/afterSave): Return value instead of Parse.Op for nested fields (parse-community#7005)
  fix(beforeSave): Skip Sanitizing Database results (parse-community#7003)
  Fix includeAll for querying a Pointer and Pointer array (parse-community#7002)
  Init (parse-community#6999)
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #7051 (0050f7a) into master (b398894) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7051      +/-   ##
==========================================
- Coverage   93.86%   93.84%   -0.03%     
==========================================
  Files         169      169              
  Lines       12437    12425      -12     
==========================================
- Hits        11674    11660      -14     
- Misses        763      765       +2     
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.84% <0.00%> (-0.75%) ⬇️
src/GraphQL/transformers/query.js 82.95% <0.00%> (-0.20%) ⬇️
src/GraphQL/transformers/mutation.js 93.47% <0.00%> (-0.08%) ⬇️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js 97.75% <0.00%> (-0.05%) ⬇️
src/GraphQL/loaders/parseClassTypes.js 94.19% <0.00%> (-0.04%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.93% <0.00%> (-0.02%) ⬇️
src/Adapters/Storage/Mongo/MongoTransform.js 88.80% <0.00%> (-0.02%) ⬇️
src/Adapters/Auth/qq.js 87.50% <0.00%> (ø)
src/Adapters/Auth/ldap.js 94.00% <0.00%> (ø)
src/Adapters/Auth/line.js 100.00% <0.00%> (ø)
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b398894...0050f7a. Read the comment docs.

Copy link
Member

@Moumouls Moumouls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it will be better :)

@dplewis
Copy link
Member

dplewis commented Dec 8, 2020

Recommending users to write test cases and link the the Contribution Guide would help.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 8, 2020

@dplewis Test cases and contribution guide would be a good addition to the bug report template indeed; did you mean also to the feature suggestion template?

@TomWFox The new template intends to drive conceptual feature discussions to the Parse Forum first, before cluttering the GitHub issue list with fundamental discussions. That means that feature discussions will be fragmented, although linked, between the Forum article and GitHub issue. I think that was also your suggestions some time back. Do you still see it that way and do you have a suggestion how to mitigate fragmented discussions between Forum and GitHub, e.g. closing a Forum article once a GitHub feature has been created? I am asking because I remember some instances in which the discussion was still going in parallel which made it difficult to consolidate.

@dplewis
Copy link
Member

dplewis commented Dec 8, 2020

Oh nevermind. LGTM!

@mtrezza
Copy link
Member Author

mtrezza commented Dec 8, 2020

@dplewis I added a test case chapter and contribution guide link to the bug template, would you take a look and see if that's what you had in mind?

@mtrezza
Copy link
Member Author

mtrezza commented Dec 8, 2020

Now that there is the GitHub Discussion feature, i wonder if we should use that as a fixed gatekeeper for feature suggestions instead of the Forum?

image

@mtrezza mtrezza changed the title Improve feature suggestion template Improve feature suggestion & bug report template Dec 8, 2020
@dplewis
Copy link
Member

dplewis commented Dec 8, 2020

Now that there is the GitHub Discussion feature, i wonder if we should use that as a fixed gatekeeper for feature suggestions instead of the Forum?

@mtrezza Open a discussion in the forum about using Github Discussion instead of the forum for discussions.

Jk

@TomWFox
Copy link
Contributor

TomWFox commented Dec 9, 2020

@TomWFox The new template intends to drive conceptual feature discussions to the Parse Forum first, before cluttering the GitHub issue list with fundamental discussions. That means that feature discussions will be fragmented, although linked, between the Forum article and GitHub issue. I think that was also your suggestions some time back. Do you still see it that way and do you have a suggestion how to mitigate fragmented discussions between Forum and GitHub, e.g. closing a Forum article once a GitHub feature has been created? I am asking because I remember some instances in which the discussion was still going in parallel which made it difficult to consolidate.

Would the goal be to weed out some feature requests to reduce the load on the issue list or just move the discussion part to the forum? If it's the latter I'm not sure what the benefit is?

I had thought we could use the forum for all feature discussions leaving only bugs reports in GitHub issues. My main concern with this is the potential of losing some of the useful discussion that happens on GH as some contributors don't really use the forum. That could well change if it was the definitive place for feature requests / discussions.

Now that there is the GitHub Discussion feature, i wonder if we should use that as a fixed gatekeeper for feature suggestions instead of the Forum?

I think this could be the right way to go, that way we can reduce the load on the issue list but keep feature discussion in GH. I'm happy to give it a go and we can back track if necessary!

@mtrezza
Copy link
Member Author

mtrezza commented Dec 11, 2020

If we activate GitHub discussions, we have FRs in 3 places:

  • GitHub issues
  • Community Forum
  • GitHub discussions

I'd say that ideally we want FRs in:

  • GitHub issues if they are actively being worked on
  • GitHub discussions if they are not being worked on or are still in a conceptual discussion phase

To ensure this, the new flow could be:

  1. Submit a FR by creating a new GitHub issue. GitHub discussions do not support templates, so we wouldn't want to start a FR there. Submitting a FR in GitHub issues is the status quo, so we keep an entry point that is familiar to authors.
  2. If the FR is still in a conceptual phase and will likely require an explorative discussion, we move it to GitHub discussions. Upon coming to a conceptual conclusion for the FR, the author can open a new GitHub issue that is specific to the concluded concept. Therefore it makes sense that the author fills out the template again.
  3. As long as a FR is actively being worked on, it stays in GH issues.
  4. If a FR is inactive for 90 days, we move it to GitHub discussions.

A time period of 90 days would currently remove 70% of open features & enhancements. After 30 days of inactivity, the "up for grabs" label is applied, so the issue stays visible for 60 days with "up for grabs". That is also the time frame in which issues are usually visible on the first issue page, before moving to the second page. If >3 months of front-page visibility do not kick-start a FR, we may as well "archive" it in discussions.

Converting an issue to a discussion is (currently) a destructive operation as discussions cannot be converted back to issues, so we rather wait until it's unlikely that it will be picked up in its proposed form. Looking at some recent FRs that have been picked up after months / years, the approach has often changed and much of the previous discussion became less relevant. So opening a fresh issue helps for clarity anyway.

More thoughts:

  • If this flow works well, we may close the Wishlist section in the Community Forum, which does not get much traffic anyway.
  • Having only bugs in GitHub issues and FRs exclusively in GitHub discussions does not seem practical. GitHub discussions serves a different purpose and brings a different feature set, most notably there are no labels, no opening/closing of discussions.

If no objections, I will go ahead and activate the discussion feature with a single category "Feature / Enhancement" and test-convert some old issues in the GitHub issue list.

@mtrezza mtrezza changed the title Improve feature suggestion & bug report template Improve templates Dec 12, 2020
@mtrezza mtrezza changed the title Improve templates Improve issue templates & add PR template Dec 12, 2020
@mtrezza
Copy link
Member Author

mtrezza commented Dec 12, 2020

I have also added a PR template.

  • Requires an issue to be opened before a PR, which gives it more visibility than PR-only and should facilitate discussion.
  • Adds a TODO-list by default of tasks to complete before merging, notably test cases and docs change.

Any thoughts?

@Moumouls
Copy link
Member

Yeah i'm totally in favor of moving FR to discussion depending of the activity on it and after a stale time. Currently it seems that the first issue page contain 90% of FR. We will have better view on real issues and we will be able to continue to discuss on proposals 😄

Also on our FR template what do you think about adding a checkbox "I'm open to start a PR on it", since the Parse Team is mainly composed of volunteer people, i think it could help us to focus a little bit more on FR that can be achieved by a new contributor 😉

@mtrezza
Copy link
Member Author

mtrezza commented Dec 12, 2020

Currently it seems that the first issue page contain 90% of FR.

Nope, ~70% or FRs are currently not on the first page.

adding a checkbox "I'm open to start a PR on it"

Not in favor, because as a principle we do not ask authors to agree to any vague future commitments that they may not want / be able to keep anyway.

Procedurally, we want to encourage a discussion about the concept before discussing a specific implementation. We've had a couple of PRs recently that were opened without issue or at the same time as the issue. This made the discussion inefficient and fragmented between concept analysis and solution debugging.

The more effective process would be:

  1. Open issue, analyze and agree on concept
  2. Open PR, debug implementation

Empirically, it shows that agreeing to start a PR initially often does not result in following through. Also, the solution and therefore the scope and complexity of a PR is still unclear pre-discussion, so asking for any commitment at this point defies the process.

If the author already has a specific PR, they may propose it as part of the issue. If the concluded solution is the same as the PR (which is rarely the case), then the author can just open the PR. If the solution is a different one, then the author can modify their branch and after that open a PR.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 12, 2020

Currently it seems that the first issue page contain 90% of FR. We will have better view on real issues and we will be able to continue to discuss on proposals

Sorry, I think I misunderstood that earlier. Yes, it should lead to a better overview - except for the first page. Due to the 90 days window, the 1st page would still contain many FRs, which is intended to keep them active.

@mtrezza mtrezza mentioned this pull request Dec 13, 2020
@mtrezza
Copy link
Member Author

mtrezza commented Dec 13, 2020

There have been some additional changes, such as the PR template, so I'll re-request review.

I've been thinking about whether to keep the emojis in Failing Test Case / Pull Request, as to not get lost in playful visuals. Decided to keep them for now in the hope that it nudges authors to create a PR with a failing test case, which could speed up fixing bugs.

@TomWFox
Copy link
Contributor

TomWFox commented Dec 14, 2020

  1. Submit a FR by creating a new GitHub issue. GitHub discussions do not support templates, so we wouldn't want to start a FR there. Submitting a FR in GitHub issues is the status quo, so we keep an entry point that is familiar to authors.
  2. If the FR is still in a conceptual phase and will likely require an explorative discussion, we move it to GitHub discussions. Upon coming to a conceptual conclusion for the FR, the author can open a new GitHub issue that is specific to the concluded concept. Therefore it makes sense that the author fills out the template again.
  3. As long as a FR is actively being worked on, it stays in GH issues.
  4. If a FR is inactive for 90 days, we move it to GitHub discussions.

How do we ensure that the same or similar process is followed each time? I feel like there needs to be a clear definition for a FR that is in conceptual phase.

Copy link
Contributor

@TomWFox TomWFox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

@mtrezza
Copy link
Member Author

mtrezza commented Dec 14, 2020

How do we ensure that the same or similar process is followed each time?

The process is controlled by the repo member, it doesn't rely much on issue authors, so we should be good there. I think we'll need a process document for these things. I suggest we try things out in the Parse Server repo for now and see how it goes.

I feel like there needs to be a clear definition for a FR that is in conceptual phase.

Yes, I didn't come to a satisfiable definition. I would leave that up to the repo member for now because this is a rare reason for converting an issue to a discussion. It would be used for features that require a fundamental discussion such as "Built-in CMS for Parse Server" (for lack of a better example). I estimate 95% of issues will convert because of inactivity, which is the main reason we want to get them out of the issue list.

@mtrezza mtrezza merged commit f01059f into parse-community:master Dec 15, 2020
@TomWFox
Copy link
Contributor

TomWFox commented Dec 20, 2020

Sorry for delay... I see why you've gone for the issue then discussion approach because of the lack of templating etc in discussions but I think this makes the process somewhat convoluted (potentially resulting in multiple issues & discussions which can't be closed, labeled etc) and potentially reduces the benefit.

As I'm not particularly involved in feature requests & discussion I would appreciate hearing from others who are more involved (@davimacedo @dplewis @Moumouls @dblythy) about whether they see this process (link to comment) as an improvement on closing stale FRs with the up for grabs label...

@dblythy
Copy link
Member

dblythy commented Dec 20, 2020

@TomWFox I would be eager to see us use a discussion format in relation to FRs. Obviously the core teams' time is valuable and probably better spent than walking through features that may or may not be appropriate.

But I like the suggestion of re-opening the discussion around old FRs, and moving active FRs to discussions. When I tackled the 2FA, I built it off the old issue and approach from 2017, whereas had of I had the discussion, we would've moved towards @Moumouls auth improvements prior to the time spent on coding.

It could also be a good way to gauge the authors' availability / skill level in creating a PR, and create discussions around supporting them through creating the feature, rather than the current process which feels a bit like "open a FR, here's how you should do it, now go and do it". I would be more than happy to provide support for newer developers to learn how to contribute, and I think that a more friendly discussion format could enable that.

I also think it would be valuable to re-evaluate and discuss old FRs so we can move towards closing them out. I'd hope that we could use the discussion format to get more feedback from the core team around the direction that the changes are going, rather than having to submit a PR to get pointers and feedback on the code. Or should we be using draft PRs in this case? Perhaps we could encourage contributors to create a draft PR around their approach to the issue so we can get feedback prior to perfecting it for merge?

So, in summary I think it is a good improvement to try to get a feel as to how much FRs are being worked on, and to discuss approach, and get pointers and tips when working on a new feature.

dplewis added a commit that referenced this pull request Feb 21, 2021
fix tests

Postgres Support

Update parse to 2.19.0 (#7060)

Fix Prettier (#7066)

Remove cache clear on validateObjects

Improve add class if not exist

Improve modifying schema instead of clearing

Improve enforce class exists

Fix flaky Test

Release 4.5.0 (#7070)

* Release 4.5.0

* Update CHANGELOG.md

Co-authored-by: Tom Fox <[email protected]>

* Improve braking change note

* Create a breaking changes sub-section

* Add release action

Co-authored-by: Tom Fox <[email protected]>

Improve issue templates & add PR template (#7051)

* improved feature suggestion template

* added test case chapter to bug report template

* PR wording

* added PR template

* improved formatting in issue template

* removed checkbox for concept due to new GH discussions process

* improved wording

* improved PR todo list

* amended PR checklist; minor rewording

* removed duplicate wording

* add securtiy check section to contribution guide

fix PR template file location (#7074)

Optimize redundant logic used in queries (#7061)

* Optimize redundant logic used in queries

* Added CHANGELOG

* Fixed comments and code style after recommendations.

* Fixed code style after recommendation.

* Improved explanation in comments

* Added tests to for logic optimizations

* Added two test cases more and some comments

* Added extra test cases and fixed issue found with them.

* Removed empty lines as requested.

Co-authored-by: Pedro Diaz <[email protected]>

FileUpload options for Server Config (#7071)

* New: fileUpload options to restrict file uploads

* review changes

* update review

* Update helper.js

* added complete fileUpload values for tests

* fixed config validation

* allow file upload only for authenicated user by default

* fixed inconsistent error messages

* consolidated and extended tests

* minor compacting

* removed irregular whitespace

* added changelog entry

* always allow file upload with master key

* fix lint

* removed fit

Co-authored-by: Manuel Trezza <[email protected]>

Fix: context for afterFind (#7078)

* Fix: context for afterFind

* Update CHANGELOG.md

Co-authored-by: Manuel <[email protected]>

Fix max listener warning from livequery server (#7083)

* fix max listner warning

* fix

* Clean test log

Run definitions

pg fix

fix: upgrade ws from 7.4.0 to 7.4.1 (#7098)

Snyk has created this PR to upgrade ws from 7.4.0 to 7.4.1.

See this package in npm:
https://www.npmjs.com/package/ws

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade ldapjs from 2.2.2 to 2.2.3 (#7095)

Snyk has created this PR to upgrade ldapjs from 2.2.2 to 2.2.3.

See this package in npm:
https://www.npmjs.com/package/ldapjs

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade semver from 7.3.2 to 7.3.4 (#7092)

Snyk has created this PR to upgrade semver from 7.3.2 to 7.3.4.

See this package in npm:
https://www.npmjs.com/package/semver

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade uuid from 8.3.1 to 8.3.2 (#7101)

Snyk has created this PR to upgrade uuid from 8.3.1 to 8.3.2.

See this package in npm:
https://www.npmjs.com/package/uuid

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr
@mtrezza mtrezza mentioned this pull request Mar 18, 2021
18 tasks
@mtrezza mtrezza deleted the change-feature-suggestion-template branch March 31, 2021 00:25
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants