Skip to content

Conversation

douglascrp
Copy link
Contributor

I've just finished my first addon validation.
Please, let me know what you think and if I should be doing something else.

@shazada
Copy link

shazada commented Sep 13, 2016

Hi Douglas,

Let me start with Nice review!
I just have one small remark.

In your Suggestion you've stated the following:
Minor re-organisation of configuration

Can you explain this a bit more? Probably it's crystel clear for you, but I'd recommend something like:

  • Move the extension code to the default templates webscript directory (orso)
  • Write a Java backed webscript which does the runas for you or change the runas to System... I'm not sure.

@AFaust
Copy link
Contributor

AFaust commented Sep 13, 2016

The issues identified concerning no 22 and 23 (runAs) should also be explicit parts of the suggested tasks.
In this review there are issues with 4 "must" / "must not" criteria, albeit only comparatively harmless ones. We may want to include either a separate column for "pass", "pass (barely)", "fail", or include a short statement in "Notes" to explain/differentiate a non-critical issue better.

@shazada The reorganisation (based on the criteria) only addresses the location in alfresco/extension/templates/webscripts vs. alfresco/templates/webscripts

Ideally, no runAs should not be needed at all as the ability to change the logo is / should be limited to the SiteManager who already has the necessary privileges. So the web script may not need to be refactored into a Java one, and the only change may be to drop the runAs from the .desc.xml

@AFaust AFaust merged commit 3f9879f into OrderOfTheBee:master Sep 22, 2016
@AFaust
Copy link
Contributor

AFaust commented Sep 22, 2016

As discussed with @douglascrp via IRC I have merged the review as-is. Douglas will check the feedback and make updates at a later time (likely new PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants