Extension Warehouse review system not sending feedback

[eneroth3]

The feedback of the review system in extension warehouse is not working.

In my experience a plugin is usually published in a day or so after submission if it passed the review process. However, on some occasions I have waited without getting any sort of feedback after the initial confirmation email that the extension is in the review queue. In the case of my most recently submitted extension I re-submitted it completely un-altered after 9 days since I thought it had gotten lost somewhere. In just a day I got this cryptic email response:

Hello Eneroth3,

We’ve been reviewing your extension on the SketchUp Extension Warehouse. Thanks much for submitting it for review!

During the review, we found a few issues that we’d like you to address:

1. Be kind to the undo stack. This includes wrapping a batch of changes into a single operation so that a single undo will roll back the batch of changes. Also avoid any changes that would mark the file as not saved that are not due to a user action, like modifying the model right after loading a file.

Here are the notes from the reviewer:
Looks like we still have the same problem.

Extension Warehouse Review Report

Extension: Eneroth Merge Layers

Author: Julia Christina Eneroth
Version: 1.0.0
Version ID: 2f71114e-660a-459b-b42d-43e6db43eab8

ERROR: Attribute ‘ene_merge_layers’/‘to_merge’ set for # without an active operation.
Source location: C:/Users/cdeluca/AppData/Roaming/SketchUp/SketchUp 2017/SketchUp/Plugins/ene_merge_layers/main.rb:60:in `add_layer_listener’

We’d love to include your extension on our warehouse, so if you’d address these items and re-submit it, we’ll review it again and get it approved.

The link to edit your extension. [<-this was a link]

Thanks,

Extension Review Team

First of all the lack of formatting (yes, the email looks as in the forum) makes the email really hard to parse. After reading it several times (over two days) I’ve however understood that:

1. My plugin got stock in an automatic filter because it writes attributes outside of an operator.
2. Someone has manually made the comment “Looks like we still have the same problem” which indicates I was supposed to already have gotten the previous piece of information.

Besides the lack of formatting there are a few bugs and other problems here.

I never got the first automatically generated email and have never ever gotten one. I have wondered for a few years now why the email you get when submitting an extensions claims that you’ll get a mail once the extension has made it through the automatic review process. There has never been such an email!

Best case-scenario: your extension passes the review and you just think the first email was sloppy written and don’t pay much attention to it. Worst case scenario: you wait for months for any sign of feedback until you give up and never try again to submit an extension.

Even once you’ve got the feedback email there’s no way to contact the reviewer for more information. The email is send from no-reply@sketchup.com, not a proper email that you can reply to. I’ve had problems with this several times when there has been bugs I haven’t been able to reproduce. I could contact ThomThom on facebook but it feels rude to contact someone privately about work, especially late at the evening and in weekends which is when I have the time to publish my extensions. I have posted about this specific issue before but got no response.

These automatic checks are also very broad and seems to gives false positives. Now that I have managed to figure out what the email meant I can understand what kind of badly made code you want to filter out. Batch operations are obviously good and marking a model as modified without user interaction is pure evil. However this feedback isn’t relevant for my plugin.I had no idea why I was seeing this for my plugin before I found the actual error message further down the email. Yes, my plugin writes attributes outside of operators. No, it’s not unkind to the undo stack and it is certainly not marking the model as modified without user interaction.

Please, start by clearly stating this being an automatic filter, then write exactly why the extension got stuck and not until then write in plain text what could have possible caused this. “Being kind to the undo stack” is irrelevant in the case of this extension. Accusing me of marking models as modified without user interaction could be taken as an insult. As it is now it really requires a a lot of knowledge and some detective work to figure out how the filter functions and what it is intended to stop before you can understand why a plugin didn’t pass the review.

I really think these issues need attention because this discourage people from making and sharing extensions. I understand that the automatic review is really complex and always will have false positives and false negatives. But please let the developer reply to the reviewer! The way the emails are written and formatted could really need some work and the missing email is clearly a bug.

@thomthom @ChrisFullmer

[gkernan]

Julia,

2 of my plugins essentially received this same treatment. I fixed it by adding a configuration parameter “Animate Doors” once I identified what was going on.

In the ruby console run Sketchup.debug_mode = true

Once the debug was on I was able to find the error.

c:/users/public/documents/sketchup/gkware_cabmaker2/cabmaker2.rb:371: warning: Ruby operation left open: “Cab Maker”

return if (! @cfgs.animate)
definition.set_attribute(DYNAMIC, ‘onclick’, “animate("angle", 0, #{hinge_angle})”)

So now my users have to set the configuration parameter if they want to use the interact feature for DC’s

What is really interesting is if you use the Interact key with ANY DC with debug set you get the same errors. This tells me that Trimble’s own DC code wouldn’t pass the automatic tests. Talk about calling the pot black.

[eneroth3]

Did you get an automatically generated email within a day after submitting the extensions?

[gkernan]

On 2017/02/27 I got 1 automatically generated email almost immediately thanking for submitting the extension but that was all. Later when I went to my account I noticed that the extension had a published date of 2017/03/03

In the past I would receive an email stating that the extension has been published.

[eneroth3]

I’ve always gotten the email saying the extension is published. However I’ve never gotten the email saying the plugin has passed or failed the automatic review process. Have you gotten such emails?

Back when the system wasn’t automated I got manually written emails with feedback. Now I only get feedback emails in the cases someone has manually written a comment, as in the message shown above.

[gkernan]

Yes I did get a failure email

Dear Developer,

Your extension has failed our automated validation test. Please correct the following issue(s) and then resubmit your extension:

[eneroth3]

Did the email contain any manually written note (“Here are the notes from the reviewer:”) or was it completely automatically generated? I have looked through my mails and ever since I started publishing extensions in 2014 I have never gotten any feedback email with purely automatically generated content, just the one saying it’s in the review queue and the one saying it’s published. Not a single mail saying it failed the the automatic review.

[tt_su]

Hi Christina

We are aware that the process is not ideal. The current system is rather cumbersome to operate and we’re currently looking into improving it. Your post is very timely as we have been collecting information like this recently.

@eneroth3

First of all the lack of formatting (yes, the email looks as in the forum) makes the email really hard to parse.

Yup - fully agreed.

@eneroth3

1. My plugin got stock in an automatic filter because it writes attributes outside of an operator.

The undo stack message is not automatic. We have tools that monitor the undo stack as we review the extension, but the reviewing is manual and not part of any automated process upon submitting.

@eneroth3

2. Someone has manually made the comment “Looks like we still have the same problem” which indicates I was supposed to already have gotten the previous piece of information.

Hmm, maybe an email got lost in spam filters?
This is an area where we’d like to make the review status and history visible to the extension developers via a dashboard of some sort. Should make things easier in the future.

@eneroth3

Even once you’ve got the feedback email there’s no way to contact the reviewer for more information. The email is send from no-reply@sketchup.com, not a proper email that you can reply to.

Indeed - this is one of the major short-coming of the current system. Even from a reviewer’s perspective we’d like to have an easy to to communicate with the developer some times. This is high up on our list to address.

@eneroth3

These automatic checks are also very broad and seems to gives false positives. Now that I have managed to figure out what the email meant I can understand what kind of badly made code you want to filter out. Batch operations are obviously good and marking a model as modified without user interaction is pure evil. However this feedback isn’t relevant for my plugin.I had no idea why I was seeing this for my plugin before I found the actual error message further down the email. Yes, my plugin writes attributes outside of operators. No, it’s not unkind to the undo stack and it is certainly not marking the model as modified without user interaction.

The top message in our feedback is part of a canned response. In order to cope with the volume of reviews we set these up for common scenarios where we reject. Though, they certainly could be improved. I think the new dashboard and better email formatting could help in providing clearer feedback.

You mentioned that you thought this was a false positive? But you also mentioned that you write attributes outside of operations. Can you elaborate to why you think that is a false positive? Sounds to me it’s catching exactly what it’s looking for. (Do you for instance have a code snippet?)

@eneroth3

Please, start by clearly stating this being an automatic filter, then write exactly why the extension got stuck and not until then write in plain text what could have possible caused this. “Being kind to the undo stack” is irrelevant in the case of this extension. Accusing me of marking models as modified without user interaction could be taken as an insult.

I’m sorry you felt the feedback was insulting. As I mentioned it’s part of a more generic canned response. With the context of the error messages we thought it would be sufficient to identify the code that triggered.
I realize that it’s hard to tell apart a canned response and a specific one. I’ll make note of that.

I don’t have a timeline for improvements, but we are actively focusing on all the issues you mentioned here. In fact we are looking at much more.

[tt_su]

@gkernan

So now my users have to set the configuration parameter if they want to use the interact feature for DC’s

What is really interesting is if you use the Interact key with ANY DC with debug set you get the same errors. This tells me that Trimble’s own DC code wouldn’t pass the automatic tests. Talk about calling the pot black.

Yes, DC is a particular troublesome extension. It was created back before SU6. And so many extensions are hooking into it that changing anything would in all likelyhood break a large number of them. Yours included. One could argue that it was never meant to be hooked into like this, but its popularity have made many extension developers do such that. The debate to break or not to break often comes up, because we are in a situation where we’d be damned if we do, damned if we don’t.

[tt_su]

@eneroth3

I’ve always gotten the email saying the extension is published. However I’ve never gotten the email saying the plugin has passed or failed the automatic review process. Have you gotten such emails?

You email quoted above was not part of automatic failure. The automatic server tests is related to file structure and some other very basic tests. The email you got was from a reviewer, with a quote from our in-SketchUp review tool that monitor the namespaces and undo operations while we review the extension.

@eneroth3

Now I only get feedback emails in the cases someone has manually written a comment, as in the message shown above.

That’s probably heavier use of canned response in an attempt to cut down the review time.

[eneroth3]

Hi Thomas!

Glad to hear you are working on this.

@tt_su

Hmm, maybe an email got lost in spam filters?

I’ve checked my spam and there no sign of it. Unless the field that to me is labeled “Here are the notes from the reviewer” has been filled out I’m not receiving these emails at all. I have several times waited a week without any response and resubmitted an un-altered plugin just to get response at all. I even have an extension I’ve waited since last summer to get a response on.

@tt_su

You mentioned that you thought this was a false positive?

When I think of it it’s probably not a false positive but the canned response made it seem to be. My plugin was writing a user setting to an attribute without using an operator. I though the default “Properties” (or whatever it is) was fine as a operator name but I can see that a custom one is better and have updated the extension.

Again, this a communication problem. Instead of trying to mimic a manually written response I think it’s much more clear to be transparent and explain exactly what kind of test the plugin failed. This can be written as an error message, there’s no need to try to hide the technical stuff from a fellow developer.

[gkernan]

@tt_su

The debate to break or not to break often comes up, because we are in a situation where we’d be damned if we do, damned if we don’t.

Then why not reply with a warning as opposed to a failure? Seems to me that you are already breaking things.

[tt_su]

@eneroth3

I though the default “Properties” (or whatever it is) was fine as a operator name but I can see that a custom one is better and have updated the extension.

Ah, gotcha. So the scenario here is making a single standalone Ruby operation. Yea, I can see the argument of not needing the start/commit operation there. Need to ponder a bit on how we can detect that scenario reliably.

@gkernan

Then why not reply with a warning as opposed to a failure? Seems to me that you are already breaking things.

With extensions that deal with DC you mean?

[gkernan]

Yes - with extension submissions such as mine that want to interact with DC’s on click etc.

Currently the extension simply fails the test. What is happening is my script is inside an active operation and the su_dynamiccomponents script must have an observer that sees the call set_attributes and then runs a script which is also inside an active operation. There needs to be a way to tell DC that there is an existing active operation and skip adding another one.

ERROR: Attribute ‘dynamic_attributes’/‘_hasbehaviors’ set for # without an active operation.
Source location: c:/users/cdeluca/appdata/roaming/sketchup/sketchup 2017/sketchup/plugins/su_dynamiccomponents/ruby/dcclass_v1.rbe:2808:in `set_attribute’

[eneroth3]

@tt_su

Ah, gotcha. So the scenario here is making a single standalone Ruby operation. Yea, I can see the argument of not needing the start/commit operation there. Need to ponder a bit on how we can detect that scenario reliably.

Or make the response text better reflect what has been detected and then say it’s best practice to always use operators. That works too.