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

Sprint end pull request #40

Open
wants to merge 234 commits into
base: master
Choose a base branch
from
Open

Sprint end pull request #40

wants to merge 234 commits into from

Conversation

Dansuf
Copy link
Collaborator

@Dansuf Dansuf commented May 31, 2018

Improved the designer and added missing options; implemented saving of workflows; connected patient side to the database; improved charts; improved PDF output; implemented "my workflows" page; improved registration and authorisation.

javier-png and others added 30 commits April 28, 2018 15:19
Period (17.04-06.05)
Added title/description for workflow and for steps, made it possible to remove steps (added confirmatiion dialog for that, just to be sure), made first start to adding rules (including lines between steps, though I'm not yet finished with that). Lots of other tiny changes (mostly layout).
When a chart type is changed for a certain result step, it is changed for every result step to be of same type.
-Rule removal when adding level between, rule removal when removing node. Rules not yet added to the database.
-Start to getting result structure of API-call. Retrieve accessible fields from current location (useful for API-call mapping)
-Fixed bug caused by removing steps
-Rule removal when adding level between, rule removal when removing node. Rules not yet added to the database.
-Start to getting result structure of API-call (useful for result representation and rules)
. Retrieve accessible fields from current location (useful for API-call mapping)
-Fixed bug caused by removing steps
workflow page loads level 0 step and will run the Evidencio model associated with that Step.

Created a search modal for designer-end
Haven't really tested it a lot, might be buggy. Was a pain to implement due to the A part of AJAX calls. Things to fix:
- Update title/colour of rule targets each modal change
@Dammes Dammes closed this Jun 12, 2018
@Dansuf Dansuf reopened this Jun 12, 2018
]);

if(array_key_exists('file',$data))
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

*
* @param Number $workflowId
* @return Array
* @param HTTP|Request $request Post request containing a Evidencio Model Search
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply request

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

* @param Number $workflowId
* @return Array
* @param HTTP|Request $request Post request containing a Evidencio Model Search
* @return JSON Evidencio models
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 string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

* Should the workflowId be given, that workflow will be updated.
*
* @param HTTP|Request $request Post request withWorkflow data (title/description, steps, etc.)
* @param Number $workflowId
Copy link
Contributor

Choose a reason for hiding this comment

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

int

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, confusion due to Javascript

return view('myworkflows', compact('workflows'));
}

public function deleteWorkflow($id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing PHP doc

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is missing?

Copy link
Contributor

@Daanra Daanra Jun 27, 2018

Choose a reason for hiding this comment

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

Something like:

/**
* Deletes a Workflow
* @param int $id The id of the Workflow
* @return void
*/

It is also missing type hints.


if($(".add-document").is(":hidden") && appRegistration.fileList.length < appRegistration.maxFileNum)
{
$(".add-document").show();
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 not really a 'Vue' way of doing it. Use v-show instead

loadModelEvidencio() {
var self = this;

$.ajax({
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to use axios for better ajax requests. You can also add the CSRF token by default, so that you do not have to add it manually all the time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Axios is nice indeed, works well with Vue. I am still starting with it though (yesterday, actually), so I still haven't quite figured out how it deals with multiple AJAX requests with one final callback.


@section('content')

{{ _("Dear")}} {{$user->first_name}},<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be one call to _. Some languages will start with the name instead of 'Dear'.

<th width="5%"></th>
<th width="5%"></th>
<th width="5%"></th>
<th width="4%"></th>
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to make this dynamic

routes/web.php Outdated
Route::get('/usersverification', 'UsersVerificationController@index')->name('usersverification.index');

Route::get('/usersverification/download/{id}', 'UsersVerificationController@download')->name('usersverification.download');
Route::post('/usersverification/accept', 'UsersVerificationController@accept')->name('usersverification.accept');
Copy link
Contributor

Choose a reason for hiding this comment

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

It's best if you split the route file up in multiple files such that each file belongs to a single entity.

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.

7 participants