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

209: Add comment fields for translators #804

Open
wants to merge 64 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@Mte90
Collaborator

Mte90 commented Aug 26, 2017

screenshot_20170826_225814

This code save the feedback when the reviewer click to change the status.
The field is disabled if is not the user that written.

I have to improve the check for permissions but actually my implementation is very simple, a new table and is possible to leave only a feedback for translation so I am not sure if it is better to leave a comment with reply or block to changes after the submit.

Ref: #209

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 28, 2017

There are a few things I noticed:

  1. The text area seems misaligned in the screenshot above.
  2. You need to update the db version in glotpress.php otherwise the new table will not be created.
  3. There should be a separate button to save the feedback, you shouldn't have to save a translation or a/r/f one to add a note.
  4. It might make sense to insert the "by admin @" line inside the note itself when it's saved, otherwise you'll lose any previous time stamps.

It's also throwing two errors with an empty table:

  1. Notice: Undefined offset: 0 in C:\wamp64\www\wp\wp-content\plugins\glotpress\gp-templates\helper-functions.php on line 239
  2. Notice: Trying to get property of non-object in C:\wamp64\www\wp\wp-content\plugins\glotpress\gp-templates\helper-functions.php on line 226
@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 28, 2017

One other item, I'm not sure I like "feedbacks", perhaps "notes" would be better? Or something else?

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 28, 2017

Yes of course there few issues but was the first draft but I can fix them asap.
I was thinking that was more helpful to save the feedback when there is the approval to avoid another step.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 28, 2017

It's fine to save it during approval, but there should be a button to save it outside of that process as well, at least in my opinion.

I can imagine an approver going back and forth with the translator a few times before getting it just right and have to "reject" it each time seems a little harsh 😉

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 28, 2017

Good point!
I will work on that asap on all the feedbacks

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 28, 2017

Something else to think about is creating a "thing" for the "feedbacks" (maybe rename "comments"?) like there is for the translations, projects, etc. (take a look in the gp-includes/things directory).

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 28, 2017

screenshot_20170828_225556

Done everything, actually wehn you add a new comment at the end of the text in the textarea automatically add the username and the date and not create duplicate in the database.

Actually is missing only the check of the permission on saving.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

I don't like the actual workflow, go to the textarea, scroll bottom, add your text and submit and automatically there is the feedback with appended username and date. The comment will be the same entry in the db with only updated the date and the user author of the last edit.

In any case is working and probably will not to be integrated with #702

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

I'd invert the flow, put new entries at the top.

Perhaps break it up in to two sections, a read only textbox to show the stored notes and a second to write the new addition?

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

Or perhaps when you click "Add feedback" it gives you a dialog box to enter the new feedback in to?

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

Uhm yes maybe show the old as not editable text outside the textarea and when saved that content is merged

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

Perhaps only let admins edit the old text, just in case they need to remove inappropriate content.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

It's not clear to me how works the permission system in glotpress so actually I didn't implemented this part to work on the rest of stuff :-)

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

Permissions are supported through the GP::$permission object (see things/permissions.php for more detail.

Basically you can use user_can() and current_user_can() to test if a user has a specified permission to a specific object type. The permissions are store in the gp_permissions table.

So to edit existing feedback you might do something like:

GP::$permission->current_user_can( 'admin', 'feedbacks', $project_id )

But to simply add feedback you could do something like:

GP::$permission->current_user_can( 'approve', 'translation', $this->id, array( 'translation' => $this ) )

Actions include: read, write, approve, admin, delete. But are not limited to those, any action that can be stored in the permissions table can be referenced by the permission functions.

Likewise for object types.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

screenshot_20170829_145728

Ok new code with few bugfix now I will work on the permission part :-)

@@ -27,7 +27,7 @@
*/
define( 'GP_VERSION', '2.4.0-alpha' );
define( 'GP_DB_VERSION', '980' );
define( 'GP_DB_VERSION', '981' );

This comment has been minimized.

@toolstack

toolstack Aug 29, 2017

Contributor

I should have mentioned, we usually bump the number in increments of 10, don't ask me why, just the way it's been done in the past.

This comment has been minimized.

@Mte90

Mte90 Aug 29, 2017

Collaborator

fixed

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

screenshot_20170829_153344
Permissions implemented, the button label change by permission and enabled to submit both the text if admin.

$button_label = __( 'Add feedback', 'glotpress' );
?>
<?php
if ( GP::$permission->current_user_can( 'admin', 'feedbacks', $project_id ) ) {

This comment has been minimized.

@toolstack

toolstack Aug 29, 2017

Contributor

$project_id isn't defined here you have to look up the project first.

This comment has been minimized.

@Mte90

Mte90 Aug 29, 2017

Collaborator

fixed

if ( ! GP::$permission->current_user_can( 'approve', 'translation', $entry->id, array( 'translation' => $entry ) ) ) {
return false;
}
if ( GP::$permission->current_user_can( 'admin', 'feedbacks', $project_id ) ) {

This comment has been minimized.

@toolstack

toolstack Aug 29, 2017

Contributor

Same as above, $project_id is not yet defined.

This comment has been minimized.

@Mte90

Mte90 Aug 29, 2017

Collaborator

fixed

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

Would changing the format of the "entries" from "test by user @ time" to:

user @ time added:
text
 

(including the blank line at the end) be more readable?

Likewise I think splitting the button in two makes it easier to understand. One button under the old text when you are an admin to update that and the second button under the new text dedicated to adding new.

With the current structure how do I know what the update button does? If I edit both fields is one going to overwrite the other?

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

The code check what text is changed and automatically update but I changed the text of feedback as you suggested :-)

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Aug 29, 2017

For me a single button is more useful because I can maybe change the old notes to remove some of them and add a new one but when I click the button I got a refresh (ajax call that replace all the html of that section) so I can lost all the edits. With a single button the user has the suggestions that can change both of them with a single action and avoid this problem.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Aug 29, 2017

I'm not tied to either way, I try and keep buttons doing only one thing as a general rule of thumb though.

The more I look at the code though I'm convinced there needs to be a thing for the table, we don't do direct SQL calls (except in very limited circumstances) as a rule and using an object for the table would keep everything consistant.

Take a look at the gp-includes/things/ directory and the base class in gp-includes/thing.php for how to start.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Sep 3, 2017

screenshot_20170903_155633
Two buttons and create a things :-)

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Sep 5, 2017

TODO: Replace feedback with note in all the code

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Sep 5, 2017

replaced the reference from feedback to note

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Sep 11, 2017

I fixed also the code issue in my code using the phpcs ruleset but phpcs report many other issue that are not from my code but in the files that I changed.

@ocean90 ocean90 reopened this Oct 31, 2018

@yoavf

This comment has been minimized.

Member

yoavf commented Oct 31, 2018

IIRC you're using WordPress' comments for the discussions tab? Are the comments linked to the original or translation ID?

Yes, we're using WordPress comments. We're creating a shadow post in a custom post type that links to an original id. The reason we're doing this with an original id is that we thought that discussions could sometimes be effective across locales - i.e to discuss the original structure, or to get help and ideas for translating a complex term.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Oct 31, 2018

  • Why are notes linked to the original ID instead of the translation ID?

Probably a bug, in the code there are reference to use the translation id, if you saw something wrong let me know but I will do a check for it.

  • How does someone know to which translation the feedback was for if there are multiple translations for one original?

As above

  • Should it be possible to let the translator reply to feedback of a validator?

Good question that we never think of it, maybe for the next iteration we can think for this feature.

  • Should it be possible to let a translator add a note to their translation to provide additional context?

I have to do a check because I am using the admin user right now (since I started to contributed I rebuilt my working instance)

  • Have you considered using WordPress' comments for feedback? This would give us edit/delete functions in the admin, spam protection through plugins like Akismet, integration into WordPress' GDPR tools, template functions for comments and the comment form, all for free.

No, thinking of that this notes are for logged user, so they are already accepting the policy of the website and this space is only for the PTE and the author of localization.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Oct 31, 2018

Speaking of cruft in the database, don't forget to update the deletion function of an original (or if you change it a translation) to also delete the associated notes.

@ocean90

This comment has been minimized.

Member

ocean90 commented Oct 31, 2018

Probably a bug, in the code there are reference to use the translation id, if you saw something wrong let me know but I will do a check for it.

In GP_Note search for original_id.

No, thinking of that this notes are for logged user, so they are already accepting the policy of the website and this space is only for the PTE and the author of localization.

What policy? How is that related? Keep in mind that GlotPress has no "PTE", only validators and translators.

Show resolved Hide resolved gp-includes/router.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
Show resolved Hide resolved gp-templates/helper-functions.php Outdated
@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Nov 3, 2018

I changed the data saved as translation_id instead original_id that was creating issues as described and also improved the permission check.
Now check if is the author of translation or with with the capabilities for the notes action.
In that way is limited who can write to them.
I done also the other changes asked.

Missing only to delete the note referenced when the original is removed. If someone has suggestion where look for that I will do also that.
I think that I can use the hook gp_original_deleted but I am not sure if it is the right way or if it is better to change original.php on after_delete.

@toolstack

This comment has been minimized.

Contributor

toolstack commented Nov 8, 2018

Missing only to delete the note referenced when the original is removed. If someone has suggestion where look for that I will do also that.
I think that I can use the hook gp_original_deleted but I am not sure if it is the right way or if it is better to change original.php on after_delete.

No need to use the hook, just add the code to after_delete would be my suggestion.

I also agree with @ocean90 that the notes should be moved out of the meta section, I've created a branch called 209-notes with a layout that I think makes sense:

3-way-layout

There are a few other tweaks in the branch as well.

One thing I noticed is that currently saving notes is broken, the code inserts the note in to the database with a null value for the translation_id.

I also noticed that if you try and add a note to an original that does not have a translation, it fails silently. There should be an error displayed in that case.

@Mte90

This comment has been minimized.

Collaborator

Mte90 commented Nov 12, 2018

I fixed for original disabling the note editor, I am checking because is not saving the translation id but the ajax request is done.
So I am investigating and thanks for the new ui. I will upload it on my demo site :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment