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.

Sign up for GitHub

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

Use site for loading of actions via Xhr and allow client specifying view #1169

Merged
merged 4 commits into from
Aug 2, 2016
Merged

Use site for loading of actions via Xhr and allow client specifying view #1169

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ace4689
Use site for Xhr-loaded actions; allow specifying view
AFaust Jul 31, 2016
a879c40
Whitespace fix
AFaust Jul 31, 2016
9f55dff
Changes after PR review
AFaust Aug 2, 2016
261f5ae
Update to mock service
AFaust Aug 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion aikau/src/main/resources/alfresco/renderers/_XhrActionsMixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,29 @@ define(["dojo/_base/declare",
* @since 1.0.46
*/
i18nRequirements: [{i18nFile: "./i18n/_XhrActionsMixin.properties"}],

/**
* The lookup key into current item to retrieve the short name of the site in which the current item is
* located. This may be relevant to correctly obtain all item actions if evaluation of these actions is
* dependant on the site scope.
*
* @instance
* @type {string}
* @default null
Copy link

Choose a reason for hiding this comment

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

When the initial assignment is null, a String, number or a boolean it is not necessary to set it on the default annotation... JSDoc automatically figures that out.

* @since 1.0.8x
Copy link

Choose a reason for hiding this comment

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

If you change this to 1.0.79 I'll look to merge this for the next release

*/
currentItemSiteShortNameKey : null,

/**
* The name of the view for which to load actions. Depending on the backend loading actions this may
* allow a differentiation between various scopes that actions have been associated with.
*
* @instance
* @type {string}
* @default null
Copy link

Choose a reason for hiding this comment

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

As with comment above, can you remove the null please?

* @since 1.0.8x
Copy link

Choose a reason for hiding this comment

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

And as above, could you set this to 1.0.79 please?

*/
actionView : null,

/**
* Overrides the [inherited function]{@link module:alfresco/renderers/_ActionsMixin#createDropDownMenu}
Expand Down Expand Up @@ -99,13 +122,28 @@ define(["dojo/_base/declare",
var nodeRef = lang.getObject("nodeRef", false, this.currentItem);
if (nodeRef)
{
// depending on how item was loaded site can be coded in different ways
var site;
if (this.currentItemSiteShortNameKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This configuration option was put in place to allow short-circuit of the "default lookup chain" if a developer is sure enough about how site can be extracted from the current item.

{
site = lang.getObject(this.currentItemSiteShortNameKey, false, this.currentItem);
}
else
{
// default fallbacks for search and doclib data web scripts (JsNode vs. bare)
site = site || lang.getObject("site.shortName", false, this.currentItem);
site = site || lang.getObject("node.location.site.name", false, this.currentItem);
site = site || lang.getObject("location.site.name", false, this.currentItem);
}
// Generate a UUID for the response to the publication to ensure that only this widget
// handles to the XHR data...
var responseTopic = this.generateUuid();
this._xhrDataRequestHandle = this.alfSubscribe(responseTopic + "_SUCCESS", lang.hitch(this, this.onXhrData, callback), true);
this.alfPublish(topics.GET_DOCUMENT, {
alfResponseTopic: responseTopic,
nodeRef: nodeRef
nodeRef: nodeRef,
site : site,
view : this.actionView
}, true);
}
else
Expand Down
35 changes: 18 additions & 17 deletions aikau/src/main/resources/alfresco/services/DocumentService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,21 +184,10 @@ define(["dojo/_base/declare",
var nodeRef = NodeUtils.processNodeRef(payload.nodeRef),
targetNodeUri = nodeRef.uri;

// Construct the URI for the request...
var uriPart = payload.site ? "{type}/site/{site}/{container}" : "{type}/node/" + targetNodeUri;
if (payload.filter && payload.filter.filterId === "path")
{
// If a path has been provided in the filter then it is necessary to perform some special
// encoding. We need to ensure that the data is URI encoded, but we want to preserve the
// forward slashes. We also need to "double encode" all % characters because FireFox has
// a nasty habit of decoding them *before* they've actually been posted back... this
// guarantees that the user will be able to bookmark valid URLs...
var encodedPath = encodeURIComponent(payload.filter.filterData).replace(/%2F/g, "/").replace(/%25/g,"%2525");
uriPart += this.combinePaths("/", encodedPath) + "/";
}

// View mode and No-cache
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code part was a remnant from a copy&paste from onRetrieveDocumentsRequest, but the calculated uriPart was not used at all.

Copy link

Choose a reason for hiding this comment

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

Thanks for highlighting this and removing it - I'd not spotted it was not being used.

var params = "?view=browse&noCache=" + new Date().getTime() + "&includeThumbnails=true";
var params = "?view=";
params += encodeURIComponent(payload.view || "browse");
params += "&noCache=" + new Date().getTime() + "&includeThumbnails=true";

var alfTopic = payload.alfResponseTopic || topics.GET_DOCUMENT;
var url;
Expand All @@ -208,7 +197,12 @@ define(["dojo/_base/declare",
}
else
{
url = AlfConstants.URL_SERVICECONTEXT + "components/documentlibrary/data/node/" + targetNodeUri + params;
url = AlfConstants.URL_SERVICECONTEXT + "components/documentlibrary/data/";
if (payload.site)
{
url += 'site/' + encodeURIComponent(payload.site) + '/';
Copy link

Choose a reason for hiding this comment

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

Could you change the single quotes to be double quotes please to avoid JSHint errors on out automated builds

}
url += "node/" + targetNodeUri + params;
}
var config = {
alfTopic: alfTopic,
Expand Down Expand Up @@ -318,7 +312,9 @@ define(["dojo/_base/declare",
}

// View mode and No-cache
params += "&view=browse&noCache=" + new Date().getTime();
params += "&view=";
params += encodeURIComponent(payload.view || "browse");
params += "&noCache=" + new Date().getTime();

var alfTopic = payload.alfResponseTopic || topics.GET_DOCUMENT_LIST;

Expand All @@ -329,7 +325,12 @@ define(["dojo/_base/declare",
}
else
{
url = AlfConstants.URL_SERVICECONTEXT + "components/documentlibrary/data/doclist/" + params;
url = AlfConstants.URL_SERVICECONTEXT + "components/documentlibrary/data/";
if (payload.site)
Copy link

Choose a reason for hiding this comment

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

It looks like this if block is not required. Stepping through the code in the unit test it seems that we're getting the site data included twice, for example on the "develop" branch we see this request:

/aikau/service/components/documentlibrary/data/doclist/all/site/site1/documentlibrary?filter=path&size=25&pos=1&sortAsc=false&sortField=cm%3Aname&view=browse&noCache=1470142533664

...but on your branch we see this:

/aikau/service/components/documentlibrary/data/site/site1/doclist/all/site/site1/documentlibrary?filter=path&size=25&pos=1&sortAsc=false&sortField=cm%3Aname&view=browse&noCache=1470142609664

Note the additional "site/site1" after "data" and before "doclist". Any thoughts on this?

I'll continue to test my update against Share to see if it causes any issues.

Copy link
Contributor Author

@AFaust AFaust Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The fragment until (and including) data/site/site1 is consumed by the Share data web script surf-doclist.get (URL pattern /components/documentlibrary/data/site/{site}/{webscript}/{params}) while the remainder is captured as the repository web script to call. As a result, the site is contained twice if both tiers are to function correctly. Otherwise the Share-tier won't capture the site token for use in any evaluators (e.g. Surf extension modules).

The if block is the integral point of this PR.

Copy link

Choose a reason for hiding this comment

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

OK... I hadn't realised that. Well, it was working in Share, so I guess that makes sense... in which case, it will be necessary to push some changes to the mocking service (aikau/src/test/resources/testApp/js/aikau/testing/mockservices/FullDocLibMockXhr.j).

In the setupServer function the first call to this.server.respondWith needs to be updated.

So...

/\/aikau\/service\/components\/documentlibrary\/data\/doclist\/all\/site\/site1\/documentlibrary\?filter=path(.*)/,

...needs to become...

/\/aikau\/service\/components\/documentlibrary\/data\/site\/site1\/doclist\/all\/site\/site1\/documentlibrary\?filter=path(.*)/,

Copy link

Choose a reason for hiding this comment

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

The last call (for favourites) will also need updating!

{
url += 'site/' + encodeURIComponent(payload.site) + '/';
}
url += "doclist/" + params;
}
var config = {
requestId: payload.requestId,
Expand Down