Pull request reviews

Topics: Announcements
Developer
Jul 19, 2014 at 1:38 PM
Edited Jul 28, 2014 at 3:30 PM
Announcing reviews here for overview. Only final verdict is listed here, comments are under the pull requests. The "verdicts" are my personal suggestions and open to debate of course.

Please make any objections or remarks for individual pull requests at the respective pull request. General remarks about pull request reviews can of course come here. I'll bring these up on the next meeting.

"Needs feedback" mean that the pull request should be discussed, feel free to chime in. Where I new that somebody is especially knowledgeable in that area I pinged them.
  1. Setup exception if database exists
    Good to go.
  2. Extended menu permissions
    Needs some adjustments and ideally would also need the next PR to be pulled in.
  3. Dynamic permissions for all
    Good to go if we can agree on the issue being relevant.
  4. Added support for prefix in FormManager
    Needs clarification.
  5. Better Edit 4 Content Items Settings
    Not needed anymore (author himself implemented a better fix), declined.
  6. Blogs and Lists
    Needs clarification.
  7. Permission names as public constants
    Needs evaluation of use-case.
  8. PostgreSQL DBMS Support
    The PostgreSQL support idea needs evaluation. Otherwise good to go.
  9. 20142 - Multi search form
    Needs evaluation of implementation and feedback from author.
  10. Content ControlWrapper Bugfix
    Good to go.
  11. Workflow Activites Search Filter
    Minimal changes needed.
  12. Fixing IE 8 uploads
    Needs rebase that may be non-trivial due to updated jQuery plugins.
  13. Adding Container ItemsShownDefault
    Good to go.
  14. MediaLibraryPickerFieldPatch1 - Fixes 20365
    Issue needs evaluation.
  15. Prevent creation of a tenant that already exists
    Good to go.
  16. ProjectionPartRecord.ItemsPerPage Hidden ...and more
    Needs clarification.
  17. Antispam Comment Tokens empty when evaluated
    Needs a bigger solution regarding the content item creation workflow.
  18. Cleanup the msbuild Precompiled build output
    I think it's good to go but I'd like to see it explained (don't know much about MSBuild internals).
  19. Ability to Edit Media Item Source
    Needs clarification.
  20. Adding ShapeTracer Styles Last
    Needs feedback.
  21. MediaLibraryService design leads to expensive enumerations
    Needs clarification.
  22. Issues when performing operations on tags with 2nd level cache enabled
    Needs a rebase, then good to go.
  23. Documentation added to some types
    Needs changes.
  24. SQLite support
    Probably good to go, needs feedback.
  25. Content picker pull request reworked
    Needs validation of use-case.
  26. Media Library pick with double click
    Needs rebase.
  27. Added "UseCdn" to SiteSettings
    Needs some feedback, but good to go.
  28. Wildcard support - cache ignore urls
    Good to go.
  29. Slugify function
    Technically good to go but needs feedback.
  30. System.Version to Sort Resources
    Good to go.
  31. Adding tfsignore file
    Needs validation but in my opinion not something that should be pulled in.
  32. Fix packaging error message.
    Tiny change, accepted.
  33. Improvements to AntiForgeryAuthorizationFilter
    Needs feedback.
  34. Media File Name Editor
    Needs some minor changes, otherwise good to go.
  35. Session configuration + Parameters
    Change needs validation of usefulness, otherwise good to go.
  36. 20601 : fix Module features pb
    Needs feedback.
  37. SchemaBuilder more DbTypes
    Good to go.
  38. Output Cache Vary by Cookie
    Minor changes needed, otherwise good to go.
  39. Configurable mediastorage rootfolder
    Good to go.
  40. Functionality to Clone Media Items
    Needs feedback.
  41. SettingsRecipeHandler bypasses attribute that is not a property
    Good to go.
  42. Taxonomies Permissions
    Depends on decision of prior issue.
  43. Volatile fields in IVolatileToken
    Needs feedback.
  44. Newtonsoft.Json.dll version conflict
    Caused by VS bug, can be declined.
  45. CommandRecipeHandler Bugfix
    Needs enlightenment.
  46. Email Updates
    Needs clarification.
  47. Output Cache Bugfix
    Needs feedback.
  48. SQL Azure cnx failures
    Needs feedback.
  49. Permission check in CommonPartDriver
    Needs feedback.
  50. Taxonomy Autocomplete
    Needs feedback.
Developer
Jul 19, 2014 at 3:29 PM
Piedone wrote:
Setup exception if database exists

Good to go.
I made a change to this pull request and rebased it onto 1.x.

Setup exception if database exists - Updated
Developer
Jul 19, 2014 at 5:14 PM
Edited Jul 19, 2014 at 5:19 PM
Thanks. You can update existing pull requests BTW. Updated the above list with new PR.
Jul 19, 2014 at 5:26 PM
As I answered __Better Edit 4 Content Items Settings__has already been applyed by Nick (thanks) in 1.8 ????
I don't know why it is still here ??
Developer
Jul 19, 2014 at 5:34 PM
Because I've closed the pull request. It might be applied by Nick but it was still open.
Developer
Jul 20, 2014 at 4:03 PM
Piedone wrote:
Thanks. You can update existing pull requests BTW.
We can add changesets to a pull request, but once the branches are chosen, you have to cancel the PR and create a new one.
Unless I'm wrong...
Developer
Jul 20, 2014 at 4:43 PM
Edited Jul 20, 2014 at 4:49 PM
Piedone wrote:
Content ControlWrapper Bugfix
Good to go.
I made a change to this pull request and rebased it onto 1.8.x.

Content ControlWrapper Bugfix - Rebased
Developer
Jul 20, 2014 at 6:41 PM
StanleyGoldman wrote:
Piedone wrote:
Thanks. You can update existing pull requests BTW.
We can add changesets to a pull request, but once the branches are chosen, you have to cancel the PR and create a new one.
Unless I'm wrong...
I see, OK.
Developer
Jul 20, 2014 at 8:57 PM
Edited Jul 28, 2014 at 3:34 PM
My post was more than 10k characters :-).

51 . Prevent adding existing tags
Good to go.
52 . Removing Size from MediaFolder
Needs some feedback but good to go.
53 . Orphaned AwaitingActivities
Needs feedback.
54 . Adding a Password EditorShape
Good to go.
55 . Hint for AllowCustomPattern = False
Good to go.

And there it is, now more unreviewed pull requests! This is not the end of the work of course, because the "good to go" ones need to be pulled in and other either updated by their authors and/or reviewed by other developers too.
Jul 21, 2014 at 10:52 AM
I removed a badly built small one (N° 36 in your list ) and created a replacement here
https://orchard.codeplex.com/SourceControl/network/forks/CSADNT/AvoidModulesFeaturesWithNullCategory/contribution/7156#!/tab/comments.
Developer
Jul 21, 2014 at 3:51 PM
Developer
Jul 21, 2014 at 4:30 PM
Developer
Jul 21, 2014 at 9:03 PM
I'm constantly updating the above list if there are changes. Many authors have updated their PRs and are now good to go.

I've opened two Codeplex issues for better PR handling: https://codeplex.codeplex.com/workitem/27315 and https://codeplex.codeplex.com/workitem/27316
Developer
Jul 23, 2014 at 10:01 AM
Developer
Jul 23, 2014 at 2:03 PM
Jul 28, 2014 at 5:03 AM
Developer
Jul 28, 2014 at 9:56 AM
That's great that you "need" that, but there is no corresponding pull request.
Developer
Jul 28, 2014 at 12:49 PM
@infofromca That "User" MediaLibrary was my little creation. Most of the functionality is implemented in the module.

The few needed changesets for it to work were merged into 1.x. So the module will be compatible with the next release of Orchard.

Like I said in that thread you can get the functionality and the module in 1.8.x if you pull changes from here: https://orchard.codeplex.com/SourceControl/network/forks/StanleyGoldman/stanleygoldman?branch=UserMediaLibrary

I freshly rebased this onto 1.8.x so it should be easy right now.
Jul 28, 2014 at 3:27 PM
Item 29 has moved here.
Developer
Aug 30, 2014 at 8:34 PM
We're reviewing a handful every week, together with triaging dozens of bug reports. This is the maximum throughput right now.
Developer
Aug 30, 2014 at 9:07 PM
Feel free to chime in on the meeting, it's in the same time and room as the normal Community Meeting, but Thursdays.
Developer
Aug 30, 2014 at 10:09 PM
Edited Aug 30, 2014 at 10:10 PM
We need like a spreadsheet more than a forum post.

I hate that you can't sort pull requests by most recently updated...

Codeplex is really starting to smell...
Coordinator
Sep 1, 2014 at 5:37 PM
Even more, we could use tagging to mark PRs as reviewed, any other state.