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

Insert and Delete Project Classifications #236

Closed

Conversation

@sandipbhuyan
Copy link
Collaborator

commented May 23, 2018

This PR is dependent on #234 PR. This is branch aims insert classifications to the ProjectClassification table. I am facing a problem while adding the form element to the Projects/settings.html.twig. I have tried to add a classification field to ProjectType Form builder. But I am getting one error
screen shot 2018-05-23 at 2 34 23 pm
I have tried another method to insert the classification data in another page(Project Classification page). And there I am able to insert the data.

[3, "Apache License"],
[3, "Solderpad License"],
[3, "other"],
[2, "week copyleft"],

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

s/week/weak

*
*/
const classifier = [
['null', "License"],

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

why not NULL?

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan May 23, 2018

Author Collaborator

While traversing the array it is taking it as an empty field. and producing this error
screen shot 2018-05-24 at 1 26 20 am

This comment has been minimized.

Copy link
@agathver

agathver May 25, 2018

Collaborator

Use prepared statements ;)

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan May 25, 2018

Author Collaborator

I have done the changes in #234 Pull request.

[3, "MIT"],
[3, "Apache License"],
[3, "Solderpad License"],
[3, "other"],

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

Other

[17, "GNU Public License v3 or later (GPLv3+)"],
[1, "Other/Proprietary License"],
[1, "Public Domain/CC0"],
['null', "Tool"],

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

NULL ?

public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder
->add('classification',TextType::class, array('label' => 'Add project classifications', 'required' => false))

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

Required = True. It will be used in a CollectionType. If a user decides to add a classification level, he must select one

@@ -29,6 +29,7 @@ public function buildForm(FormBuilderInterface $builder, array $options)
->add('projectUrl', UrlType::class, array('label' => 'Project URL', 'required' => false))
->add('issueTracker', UrlType::class, array('label' => 'Issue/Bug Tracker URL', 'required' => false))
->add('sourceRepo', SourceRepoType::class)
->add('classifications', ProjectClassificationType::class)

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

You are representing a collection of ProjectClassification. This type should be CollectionType. This is causing the error here.

$builder->add('tags', CollectionType::class, array(
            'entry_type' => ProjectClassificationType::class,
        ));
<h3 class="panel-title">Project Classification</h3>
</div>
<div class="panel-body">
{{ form_row(form.classifications) }}

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

This will not work without javascript. Don't render it. Instead construct a dropdown that you manually populate, just use the name from the form.

<select name="{{ form.classifications.name }} " ... >
   <option value="...">Text</option>
</select>

@agathver agathver removed the help wanted label May 23, 2018

@agathver

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2018

@sandipbhuyan
I think CollectionType will resolve your issue. Get this one running and then I would take a proper look at the code.

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2018

I have completed the insert feature. Its needed to be get reviewed soon as it is a blocker for the rest of the feature.

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch from 66b542a to 5e37cb1 May 31, 2018

@sandipbhuyan sandipbhuyan referenced this pull request Jun 4, 2018
2 of 2 tasks complete
@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

Please rebase this on top of the current master and run phpcs as described here http://librecores-web.readthedocs.io/en/latest/devenv/tipsandtricks.html#check-the-coding-style-of-php-code

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

And how does this PR relate to #240 and #239? Insert, update and delete of classifications on the settings page are one topic and should be handled together.

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

I am rebasing this branch and adding some extra features and updating this branch by tonight

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

I have made three different PRs for these three features for making the review process easier and faster.

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch from f312630 to 5535056 Jun 11, 2018

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch from 966d38f to 0168000 Jun 12, 2018

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

@imphil I have done some changes in this PR. Now the insertion process is done by an ajax request.
I have changed the UI a little.
screen shot 2018-06-12 at 11 23 08 am
While I am running phpcs one error is coming ERROR: Referenced sniff "Symfony" does not exist

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

While I am running phpcs one error is coming ERROR: Referenced sniff "Symfony" does not exist

you need to rebuild the VM or run composer install

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

@imphil I have done that but still, I am getting the same error. It might come from the phpcs.xml.dist file.
The second thing is I have created two different PRs for the update and delete classifications. As the lines of code added exceeding 300+ so I thought it will be better to put those two in two different PR and also we can have a better look on the modification of each of these features. I can put these three in a single PR if you want.

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

The phpcs path is now fixed in master, please give it a try.

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

Also as said before: insert, update and delete require a unified UI, this is nothing I can review in three pieces. Please make this a single PR.

It might be useful to have a series of commits inside this single PR which don't reflect the evolution of your progress but individual, standalone chunks of work (e.g. the backend work, frontend work, other unrelated changes). A rough guideline is given in the Linux Kernel documentation and applies to any other project as well: https://www.kernel.org/doc/html/v4.16/process/submitting-patches.html#separate-your-changes

You can use git rebase to update this series after a review round.

sandipbhuyan added some commits Jun 6, 2018

Remove internal javascript from settings page
Internal js in project settings page have been removed and placed inside classification.js file
Insert classification for a request by a AJAX request
    Project classifications are inserted through ajax request and the classification for a give project is being retrived and displayed on settings page for further operations

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch from 0168000 to 90eec1d Jun 12, 2018

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

@imphil I have updated this PR. delete and insert of classification is done from the settings page and I have created another page to update a project classification.

@sandipbhuyan sandipbhuyan changed the title Insert classification to database Insert, Update and Delete Project Classifications Jun 12, 2018

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

My comments as discussed in the hangouts call:

  • Remove the update tags functionality. It's adding significant complexity for little gain.
  • Make sure the added and removed tags are only saved to the database when clicking the "update project" button. You can build up the classifications before that in a session.
  • No advanced modifying of the database entity is required regarding the classifications. The new classifications can be written by simply deleting all existing ones and adding the new ones.

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch 3 times, most recently from 1742700 to c5cf156 Jun 13, 2018

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:insert-classification branch from c5cf156 to ac3f1d4 Jun 13, 2018

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2018

@imphil and @agathver I have made some changes in this PR. I have improved the following features in it

  • Remove update classifications
  • Insert or delete classification while clicking on update project.
    I have changed the UI Just have a look at it.
    screen shot 2018-06-13 at 11 43 36 pm

@sandipbhuyan sandipbhuyan changed the title Insert, Update and Delete Project Classifications Insert and Delete Project Classifications Jun 14, 2018

@imphil
Copy link
Contributor

left a comment

Thanks Sandip, good work! I like the UI and the general functionality, but I have a couple of comments marked in the code.

Additionally, I'm not a huge fan of the JS as it is right now, but pure JS tends to be ugly, so let's keep it like that for now (with the review comments fixed, of course).

In the longer term we should have a look at some frontend framework which makes such things nicer (I had good experience with vue.js recently.)

$projectClassification = new ProjectClassification();
$projectClassification->setProject($p);
$projectClassification->setClassification($classification);
$projectClassification->setCreatedBy($p->getParentUser());

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

$p->getParentUser() fails if the project is owned by an organization. And in fact we're not looking for the project owner here, but the user who actually added the classification.

// Retrive the project classifications for a project
$classifications = $this->getDoctrine()->getManager()
->getRepository(ProjectClassification::class)
->findBy(['project' => $p->getId()]);

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

Use ->findByProject($p);

foreach ($projClassification as $classification) {
$projectClassification = new ProjectClassification();
$projectClassification->setProject($p);
$projectClassification->setClassification($classification);

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

$classification is not validated in any way and could be used to insert arbitrary classifications into the DB. Please add validation. You do not need to use the Symfony validation component, simply bailing out with an Exception if invalid categories are found is sufficient (to reach that state the user must have modified the HTTP request, hence we don't need to show a nice UI).

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan Jun 19, 2018

Author Collaborator

I am a little bit confused about this part. Yes, we need validation while adding a classification but my question is what type of rules we are going to apply. In the current state, we are using a select element to specify a classification to a project. That is a kind of validation we are including at the frontend part. while saving this a classification for a project I am saving the whole string i.e LibreCores:: Featured so in my view we need to validate the two categories separately if it is so then I can write the logic for it. If you want any other type of validation then I want to know more about it.

This comment has been minimized.

Copy link
@imphil

imphil Jun 19, 2018

Contributor

The backend must check if the values passed to it by the frontend are allowed according to our rules. In this case, that means a classification must be valid according to ClassificationHierarchy. You can (should, and already do) additionally ensure in the frontend that a user can only enter valid data. But frontend-only validation is not enough: malicious actors can craft a HTTP request with arbitrary data, and use this to insert classifications into the DB which would not have been allowed in the frontend.

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan Jun 19, 2018

Author Collaborator

So I think there will be two rules

  • :: should separate the categories
  • and the categories should be in the classification hierarchy.
    If you have any other rules then let me know

This comment has been minimized.

Copy link
@imphil

imphil Jun 19, 2018

Contributor

Almost there: the categories should not only be in the classification hierarchy, but the full category should be valid according to the hierarchy.

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan Jun 19, 2018

Author Collaborator

Got it, Thanks updating the PR soon

// Retrive the project classifications for a project
$classifications = $this->getDoctrine()->getManager()
->getRepository(ProjectClassification::class)
->findBy(['project' => $p->getId()]);

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

see above

1 => $category->getId(),
2 => $category->getParent() == null ?
$category->getParent(): $category->getParent()->getId(),
3 => $category->getName(),

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

Why this strange numbering of the array starting with 1 (instead of 0)? Why don't you use named keys which would be more descriptive?

})
}

//remove a category to the project

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

"remove a classification from"

<a class="remove-classification" href="#">\
<i class="fa fa-close" aria-hidden="true"></i>\
</a>\
</div>')

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

That's a very unusual code formatting. You can continue lines by just splitting the string.

$('.delete-classifications').empty();
}
}
else if ($('.remove-classification').parent().hasClass('update')) {

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

formatting: } else if () {

<a class="remove-classification" href="#">\
<i class="fa fa-close" aria-hidden="true"></i>\
</a>\
</div>')

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

formatting: same comment as above.

var clasificationId = $(this).attr('href');
var classificationName = $(this).siblings('span').html();
$(this).parent().hide();
if ($('.delete-classifications').children().length <= 0) {

This comment has been minimized.

Copy link
@imphil

imphil Jun 18, 2018

Contributor

can length be smaller than 0?

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2018

@imphil I have done some changes in this PR. Have a look at it when you are free. I have modified the UI a little. Now the classification error will be displayed like this
screen shot 2018-06-21 at 10 01 42 pm

@imphil
Copy link
Contributor

left a comment

Thanks for updating this PR, we're now very close. I have mentioned two minor things in the code. Additionally, I have found a small problem with escaping somewhere.

Do the following:

  • Modify the database to change the permissive entry to pe'rm>iss"ive by executing this SQL statement in phpMyAdmin (pma.librecores.devel): UPDATE ClassificationHierarchySETname= 'pe\'rm>iss"ive' WHEREClassificationHierarchy.id = 3;
  • Go to the project settings page and try to insert a classification containing this version of "permissive".
  • Click on "Update project".

The following error message is shown:
Invalid classifications - License::Free and Open::pe'rm>iss

So somewhere in the process the string prematurely ends at the " sign, most likely because " must be escaped but isn't.

With those things fixed I think this can be merged.

}
if (++$i == $count) {
return true;
}

This comment has been minimized.

Copy link
@imphil

imphil Jun 25, 2018

Contributor

You don't need the count here. It should be sufficient to return true after the end of the foreach loop (and leave the "return false") where it is currently. Even more understandable code could be created by returning early, something like that:

foreach ($categories as $category) {
    if (!array_key_exists($category, $classifications)) {
        return false;
    }

    // now load subcategories
    // ...
}

return true;
var count = 1;
for (var i = 0; i < classificationDetails.length; i++) {
if (classificationDetails[i]["parentId"] === null) {
$('#category-' + count + '').append($("<option>").val(classificationDetails[i]["name"]).html(classificationDetails[i]["name"]));

This comment has been minimized.

Copy link
@imphil

imphil Jun 25, 2018

Contributor

the + '' is unnecessary. (Same a couple times below.)

And please stay with one style of string delimiters (' or ")

And break the line to get somewhere closer to 80 characters.

@imphil imphil closed this in 51d5169 Jun 28, 2018

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

I fixed some typos and merged it as a squashed version. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.