Proposed changes

closes #6440

• Added a Delete entry button for removing translation suggestion from Automatic Suggestions

Checklist

• [x] Lint and unit tests pass locally with my changes.
• [x] I have added tests that prove my fix is effective or that my feature works.
• [x] I have added documentation to describe my feature.
• [x] I have squashed my commits into logic units.
• [x] I have described the changes in the commit messages.

Aug 09 '22 10:08 ParthS007

Codecov Report

Merging #7984 (b77b201) into main (e92cb03) will increase coverage by 0.12%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7984      +/-   ##
==========================================
+ Coverage   90.82%   90.95%   +0.12%     
==========================================
  Files         685      685              
  Lines       51251    51256       +5     
  Branches     7400     7934     +534     
==========================================
+ Hits        46550    46619      +69     
+ Misses       3305     3248      -57     
+ Partials     1396     1389       -7     

Aug 12 '22 00:08 codecov[bot]

@nijel One thing that we discussed earlier for multiple origin suggestions, Should I add multiple delete buttons for them or an Icon with delete functionality would be more intuitive.

In the later case, then instead of delete button, we can have delete icon just next to origin instead of a Delete entry button.

What do you think would be good to go?

For example

Aug 12 '22 12:08 ParthS007

We definitely need a way to delete it in selected origins only. I can see two approaches:

• Move the delete action to the origin column, probably making it icon only. This will pollute the UI quite a lot.
• Add confirmation dialog which would list all origins with checkboxes. That way the UI is consistent, and it is possible to uncheck origins which the user doesn't want to delete.

That reminds me cases which is not addressed in this PR yet:

• The code currently picks only first delete URL, and thus it always deletes first origin only.
• In case the matching string is first obtained from other sources (machine translation service), the delete button will not be present, even if later the suggestions from the translation memory arrives.

The preferred solution as I see it now (feedback welcome):

1. Collect the delete URLs at origin element.
2. Once there is at least one delete URL, display the Delete button in the row.
3. Make the Delete button open a popup listing all origins for current translation which have the delete URL. All selected by default.
4. The callback would then fire DELETE on all selected URLs.

Aug 15 '22 09:08 nijel

The solution sounds good to me however it will result in more clicks required to delete an entry, imo it's not good for UX. Incase of the delete icon, the number of clicks remain same and user can just click on the delete icon of entries.

I am not sure what do you mean here by:

This will pollute the UI quite a lot.

I am okay with implementing the preferred solution though. Please point me to any similar popup implementations if we have any just in case.

wdyt?

Aug 16 '22 13:08 ParthS007

I think we want confirmation here anyway - having a delete button next to copy without a confirmation doesn't sound good. The confirmation popups are used in several places, for example here:

https://github.com/WeblateOrg/weblate/blob/c8f507972102558803f29abb0f1cfacd048c3a96/weblate/templates/trans/project-access-row.html#L44-L59

Aug 17 '22 14:08 nijel

@nijel I worked on the proposed solution. Apologies for the delay.

The preferred solution as I see it now (feedback welcome):

• [x] Collect the delete URLs at origin element.
• [x] Once there is at least one delete URL, display the Delete button in the row.
• [x] Make the Delete button open a popup listing all origins for current translation which have the delete URL. All selected by default.

The popup looks like this

• [x] The callback would then fire DELETE on all selected URLs.

I am able to callback the DELETE request but the request gets blocked by firefox (I am not sure what;s the reason, might be related to csrf token or something else) and I get the following error notification

Sep 09 '22 21:09 ParthS007

I've tested that and I can see few issues:

1. The UI looks weird when the delete button is not shown:

   

   I suggest adding empty cell instead.

2. The delete button is rendered for some services where it should not be present:

   

   Clicking on it yields empty dialog

3. The dialog is not cleared before display, so when you abort it, and show it again, it includes duplicate entries:

   

Sep 15 '22 07:09 nijel

Thanks for review, I could reproduce 1 and 3.

1. The empty cell screenshot if delete button is not present.

2. I couldn't reproduce 2, I am assuming 2 might have happened because of inconsistent state so I have simplified the logic for showing the delete button. I tried on multiple pages and cannot see unexpected delete button.

3. The modal screenshot after closing it several times, no duplicate entries present

Sep 18 '22 13:09 ParthS007

Thanks, looks better. There is still some issue with the dismissed popup - the dialog shows three checkboxes, but the DELETE request is fired multiple times (it seems that each dismiss adds one repetition):

Sep 19 '22 06:09 nijel

@nijel Please let me know if there is something left for this PR. Thanks

Sep 26 '22 07:09 ParthS007

Tests passing 🎉

Sep 27 '22 07:09 ParthS007

Merged, thanks for your contribution!

Sep 27 '22 09:09 nijel