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

Suite's history #328

Merged
merged 42 commits into from
Aug 29, 2018
Merged

Suite's history #328

merged 42 commits into from
Aug 29, 2018

Conversation

m-suchorski
Copy link

User can now view and go to any version of that suite.

Description

User is now able to go trough all versions of the suite by clicking on the timestamp.
Suite's versions are displayed after clicking the current version on the top toolbar.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

I hereby agree to the terms of the AET Contributor License Agreement.

Copy link
Contributor

@tkaik tkaik left a comment

Choose a reason for hiding this comment

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

Please update Changelog

/**
* @param dbKey - key with project and company name
* @param name - name of suite
* @param version Suite object found by given criteria or null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this javadoc - move Suite object found by given criteria or null. to @return and add description for version param

@@ -143,6 +157,21 @@ public Suite apply(Document result) {
}).toList();
}

public List<String> listSuiteVersions(DBKey dbKey, String name) throws StorageException{
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should return list of pairs: { version: "..", correlationId: "..} - we can't assume that oldest suite will have version equal to 1 because Cleaner might have deleted it

return 'http://aet-vagrant' + '/api' + '/history' + '?' +
$allParametersList[0] + '&' +
$allParametersList[1] + '&' +
$allParametersList[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

The path is wrong when correlationId is used.
Example: /report.html?company=xxx&project=xxx&correlationId=xxx

.find(Filters.eq(SUITE_PARAM_NAME, name))
.sort(Sorts.descending(SUITE_VERSION_PARAM_NAME));

return FluentIterable.from(found).transform(new Function<Document, String>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may use Java 8 features instead, e.g.:

StreamSupport.stream(found.spliterator(), false)
        .map(result -> result.getString("correlationId"))
        .collect(Collectors.toList());

.collect(Collectors.toList());
}

public List<List<String>> listSuiteVersions(DBKey dbKey, String name) throws StorageException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should return List<Map<String, String>> or List<SuiteVersion> (where SuiteVersion is a new class with 2 fields - version and correlationId. Then in HistoryServlet you just need to call gson.toJson instead of the whole JsonArray/JsonObject building

Copy link
Contributor

@tkaik tkaik left a comment

Choose a reason for hiding this comment

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

Please update Report app documentation with new Suite features

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Please update also report documentation with example of using history.

updateToolbar();

/***************************************
*********** Private methods *********
***************************************/

function updateToolbar() {
document.getElementsByClassName('suite-history-container')[0].style.display = 'none';
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use jQuery here.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

};
}
}

function buildApiPath($allParametersList) {
return 'http://aet-vagrant' + '/api' + '/history' + '?' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use AET endpoint address defined in the endpointConfigurationService.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

$allParametersList[2];
}

function getSuiteHistory(suiteHeaders, $rootScope, $http) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of logic should be placed into angular service not in the controller.
E.g. look at metadataEndpointService.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -83,5 +86,31 @@
<div class="toolbar-block fontawesome-link" data-toggle="modal" data-target="#helpModal">
<i class="fas fa-question-circle fa-lg"></i>
</div>
<div class="suite-history-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good idea to put that into separate modal? Like e.g. noteModal (separate controller and view).

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -75,7 +76,11 @@ protected void process(DBKey dbKey, HttpServletRequest req, HttpServletResponse
if (isValidCorrelationId(correlationId)) {
suite = metadataDAO.getSuite(dbKey, correlationId);
} else if (isValidName(suiteName)) {
suite = metadataDAO.getLatestRun(dbKey, suiteName);
if(isValidName(suiteVersion)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be method isValidVersion here?

* **URL**: `/api/history`
* **HTTP Method**: GET
* **Parameters**: `company`, `project`, `suite`
* **Example**: http://aet.example.com/api/history??company=cognifide&project=example&suite=mysimplesuite
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix typo - double ?


![Suite history](assets/suiteReport/suite-history.png)

Clicking the suite's name in the top toolbar will show a popup in which the user can see every suite's version that is still available in the database. User can also directly go to the newest version of the suite by clicking
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like unfinished sentence :)

Copy link
Author

Choose a reason for hiding this comment

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

Right, I don't know what happened there. Fixing now.

*/
function historyService($rootScope, $http, endpointConfiguration) {
var suiteHeaders;
$rootScope.endpointUrl = endpointConfiguration.getEndpoint().getUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that necessary to bind this value to the $rootScope variable?
Maybe you can use local variable instead?

Copy link
Author

Choose a reason for hiding this comment

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

This value isn't used anywhere in the service file - it is used only in the view file so it has to be put in the $rootScope, otherwise it wouldn't be possible to use it in the historyModal.view.html file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where exactly in historyModal.view.html is used endpointUrl?

suite = metadataDAO.getLatestRun(dbKey, suiteName);
if(isValidVersion(suiteVersion)){
suite = metadataDAO.getSuite(dbKey, suiteName, suiteVersion);
}else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix formatting here

var suite = cUrl.searchParams.get('suite');
if (suite === null) {
var correlationId = cUrl.searchParams.get('correlationId');
suite = correlationId.split('-')[2];
Copy link
Contributor

@tkaik tkaik Aug 23, 2018

Choose a reason for hiding this comment

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

It is possible that company or project or suite values contain - character - then this line will not work correctly.
I think it can be done like this (probably can be done simpler :))

var cp = company + '-' + project + '-';
var suiteNameWithTimestamp = correlationId.substr(a.indexOf(cp) + cp.length)
// e.g. now: suiteNameWithTimestamp == 'suite-name-123123123` - we need to cut timestamp from here

var company = suiteHeaders[i].correlationId.split('-')[0];
var project = suiteHeaders[i].correlationId.split('-')[1];
var suite = suiteHeaders[i].correlationId.split('-')[2];
var correlationId = suiteHeaders[i].correlationId.split('-')[3];
Copy link
Contributor

@tkaik tkaik Aug 23, 2018

Choose a reason for hiding this comment

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

I think that you're parsing timestamp from the correlationID here, so this var should be renamed to e.g. timestamp
And as above - we should handle cases where company/project/suite have '-' characters

var company = company;
var project = project;
var suite = suite;
var timestamp = suiteHeaders[i].correlationId.split('-')[suiteHeaders[i].correlationId.split('-').length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit hard to read now, can we make suiteHeaders[i].correlationId.split('-') a variable? e.g.:

var correlationIdParts = suiteHeaders[i].correlationId.split('-');
var timestamp = correlationIdParts[correlationIdParts.length - 1]

project: project,
suite: suite,
version: version,
correlationId: timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

correlationId: timestamp is a bit confusing, please change to timestamp: timestamp (because it's not correlatoinId here)

@tkaik tkaik merged commit f9460fd into master Aug 29, 2018
@m-suchorski m-suchorski mentioned this pull request Sep 11, 2018
6 tasks
@wiiitek wiiitek deleted the feature/suite-history branch October 13, 2018 13:42
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.

4 participants