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

Add option to purge only deleted messages #565

Merged
merged 21 commits into from
Apr 6, 2020

Conversation

multiwebinc
Copy link
Contributor

@multiwebinc multiwebinc commented Mar 25, 2020

Related bug report: #525

I've added another checkbox in the scanner window that when checked will purge only the messages that the scanner was unable to find. For the life of me I couldn't figure out how to add a column to the list view to indicate that the string couldn't be found in the project anymore to give the translator an idea which strings can be safely deleted.

It is also missing a migration to add a boolean column to rainlab_translate_messages called found. I'm not exactly sure how that should be handled since you need to increment the version.

@mjauvin
Copy link
Contributor

mjauvin commented Mar 25, 2020

Thanks a lot!
I'll take a look on how we could add those columns to the messages list.

@mjauvin
Copy link
Contributor

mjauvin commented Mar 25, 2020

I had a quick look and I didn't see where the messages actually get purged... what am I missing?

@multiwebinc
Copy link
Contributor Author

I missed uploading the controller.

models/Message.php Outdated Show resolved Hide resolved
@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

@multiwebinc for the migration, just create the update_messages_table.php migration file and I'll take care of the version.yaml file.

@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

update_messages_table.php:

<?php namespace RainLab\Translate\Updates;

use Schema;
use October\Rain\Database\Updates\Migration;

class UpdateMessagesTable extends Migration
{
    public function up()
    {   
        Schema::table('rainlab_translate_messages', function($table)
        {   
            $table->boolean('found')->default(1);
        });
    }       

    public function down()
    {   
        Schema::table('rainlab_translate_messages', function($table)
        {   
            $table->dropColumn('found');
        });
    }
}

models/Message.php Outdated Show resolved Hide resolved
@mjauvin
Copy link
Contributor

mjauvin commented Mar 27, 2020

LGTM! I ran a few tests and it seems to be working as expected!

@mjauvin mjauvin self-assigned this Mar 27, 2020
@mjauvin mjauvin linked an issue Mar 27, 2020 that may be closed by this pull request
@multiwebinc
Copy link
Contributor Author

@mjauvin How hard would it be to put a small exclamation mark type icon on the message list page for translations marked as not found? I couldn't easily figure that out the other day.

@mjauvin
Copy link
Contributor

mjauvin commented Mar 27, 2020

The markup for this is rather alien to me, couldn't figure it out either.

models/Message.php Outdated Show resolved Hide resolved
@mjauvin
Copy link
Contributor

mjauvin commented Mar 27, 2020

@multiwebinc I am slowly making progress at adding a column to show the "found" db field...

@mjauvin
Copy link
Contributor

mjauvin commented Mar 27, 2020

@multiwebinc I'll let you play with this, but so far I did the following:

In controllers/messages/config_table.yaml modify columns as below:

columns:
    from:
        title: From
    to: 
        title: To
    found:
        title: Found
        type: checkbox
        width: 100px

And in controllers/Messages.php add the found column data in processTableData() as below:

$data[] = [
    'id' => $message->id,
    'code' => $message->code,
    'from' => $message->forLocale($fromCode),
    'to' => $toContent, 
    'found' => $message->found
];

checkbox column type should be able to be marked "readOnly" but it's not working.

It's currently rendering as below:

Screenshot from 2020-03-27 18-29-28

@multiwebinc
Copy link
Contributor Author

@mjauvin I couldn't find any documentation for the backend table widget, however the supported column types are "string", "checkbox", "dropdown" and "autocomplete". These are all rendered client-side with JS and the data is pulled using AJAX. If the "string" column type is chosen, it is HTML entity encoded, so it doesn't look like there's a straightforward and easy way to do what I was thinking. Using a read-only checkbox would be my second choice, however the read-only attribute has no effect for the checkbox column type. In fact, selecting/unselecting the checkbox does not update the value in the database either. Looks like a possible bug to me.

@multiwebinc
Copy link
Contributor Author

Ok, this is how it is rendering now. I put the info icon with a title attribute explaining what this means in the table header:

image

@mjauvin
Copy link
Contributor

mjauvin commented Apr 1, 2020

@multiwebinc I finally had time to test this and it works well.

I think the column header would make more sense being "notes" rather than "found" since the later sounds more like it should say it was found (yes) or not (no)... only marking the records that have not been found is confusing when using that column header in my opinion (especially if there are no missing translations).

Other that that, it looks great!

@multiwebinc
Copy link
Contributor Author

@mjauvin The thing is that if it wasn't found with the scanner, I would like to be able to provide a slightly more descriptive feedback to the user than just "Not found" since I would guess the majority of users wouldn't know what that means exactly. What if we call it "Scan errors", or "Scan result"?

@mjauvin
Copy link
Contributor

mjauvin commented Apr 1, 2020

@mjauvin The thing is that if it wasn't found with the scanner, I would like to be able to provide a slightly more descriptive feedback to the user than just "Not found" since I would guess the majority of users wouldn't know what that means exactly. What if we call it "Scan errors", or "Scan result"?

Whatever makes sense, I was just saying "Found" didn't make sense here.

@multiwebinc
Copy link
Contributor Author

@mjauvin Take a look now. Let me know if the wording sounds good to you now.

@mjauvin
Copy link
Contributor

mjauvin commented Apr 2, 2020

Looks good @multiwebinc!

@mjauvin mjauvin merged commit 64d76ce into rainlab:master Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow purging messages to keep existing translations
3 participants