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

issue-#895 #2379

Merged
merged 18 commits into from
Jul 29, 2020
Merged

issue-#895 #2379

merged 18 commits into from
Jul 29, 2020

Conversation

bakananbanjin
Copy link
Contributor

@bakananbanjin bakananbanjin commented Jun 4, 2020

this should close #895
its me again, I made some more adjustments. For starts I show now all public lists and own lists instead of just the users own lists. I think its for the user more interesting to see in which (public) lists the sentences is. he only can remove them if he got the remove right. also the options to remove sentences are only visible for members.

other than that no changes in the old functionality. I changed according to your @AndiPersti suggestions.
except the first because i didnt knew what you ment

What do you mean?
I can only see that the space above the Lists heading got bigger which I don't think is a good idea because now it doesn't match the other modules in the sidebar any more.

ok still looking forward to hear from you guys what you think.

(and one more question how does i update a pull request if i make further changes? just press the button again?)

everyone who goes to bed now like me I wish a good night everyone else a nice day morning and evening XD

@bakananbanjin bakananbanjin changed the title update issue-#895 Jun 4, 2020
@AndiPersti
Copy link
Contributor

Thanks for cleaning up the PR 👍

For starts I show now all public lists and own lists instead of just the users own lists.

I'm not sure whether that's a good idea. #1800 discusses some privacy issues related to showing public lists on the sentence page.

except the first because i didnt knew what you ment

In your old version you've added an additional div which added some whitespace above the heading Lists. This isn't the case anymore so you can ignore this comment.

But I can see now, that you've actually changed the design of the tag module by listing each tag on its own line. As with the displaying of all public lists, I think that needs further discussion.

In general I would suggest to focus on the specific issue and don't mix in further changes if they aren't necessary to fix the problem.
Especially changing the UI is a sensitive issue because it will quite often annoy some users. 😄

(and one more question how does i update a pull request if i make further changes? just press the button again?)

You just need to git push the changes to your branch. The PR will be updated automatically.

@bakananbanjin
Copy link
Contributor Author

I'm not sure whether that's a good idea. #1800 discusses some privacy issues related to showing public lists on the sentence page.

I see, I still think if someone has put his list on public its public information. But I am not here to decide it. Just in my opinion for usersit could be beneficial to see all list with this sentences.

But I can see now, that you've actually changed the design of the tag module by listing each tag on its own line. As with the displaying of all public lists, I think that needs further discussion.

Same argument then above, but also your call. I think you will discuss it with the other developers. Just let me know.

The changes are all minor, so if you have decided how to proceed, i will change it. Other then that looking forward for your feedback and for more work to do XD

src/Controller/SentencesController.php Outdated Show resolved Hide resolved
src/Controller/SentencesListsController.php Outdated Show resolved Hide resolved
src/Controller/SentencesListsController.php Outdated Show resolved Hide resolved
src/View/Helper/ListsHelper.php Outdated Show resolved Hide resolved
src/View/Helper/ListsHelper.php Outdated Show resolved Hide resolved
src/View/Helper/ListsHelper.php Outdated Show resolved Hide resolved
src/View/Helper/ListsHelper.php Outdated Show resolved Hide resolved
Comment on lines 613 to 618
} else
{
echo '<div class="section md-whiteframe-1dp">';
/* @translators: header text on the sidebar of a sentence page */
echo $this->Html->tag('h2', __('Lists'));
echo '</div>';
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the UI by displaying now an empty Lists block on all sentence pages when the sentence doesn't belong to any list. I'm not sure if that is what we want.

docs/database/tables/sentences_lists.sql Outdated Show resolved Hide resolved
src/View/Helper/ListsHelper.php Outdated Show resolved Hide resolved
@bakananbanjin
Copy link
Contributor Author

@AndiPersti Thanks for the time your time to look over my code and all the advises. The simple ones I already changed. (PR request if I done everything, maybe tomorrow) I added some few questions for clarification.
For now I sign off, cu

@AndiPersti
Copy link
Contributor

I added some few questions for clarification.

I don't see any questions.

@bakananbanjin
Copy link
Contributor Author

@AndiPersti hi i meant I commented on your comments. wwww

@bakananbanjin
Copy link
Contributor Author

bakananbanjin commented Jun 8, 2020

Ok now most should be resolved. (the space between x and listname i forgot, but push it in a min).
1.
I decided for now that if the list is public+ and its editable by anyone that i still show the x. But if the list is public+ and only editable by its creator I dont show the delete button. But if you think I should only show it by the users list, let me know and I change it:

`if($list['user_id'] == $currentUserId || $list['editable_by'] == 'anyone')
                 {
                    echo $this->_displayRemoveLink($list['id'], $sentenceId, $list['name']);
                 }

If the sentence is in no List i still show the UI Module, because I wasnt sure if you think it should get deleted. w8 for your response.

This changes the UI by displaying now an empty Lists block on all sentence pages when the sentence doesn't belong to any list. I'm not sure if that is what we want

  1. Ok now we come to the main problem, the migration file. I am quite sure that I could write it from hand, but wanted to use
    cake migrations create MyNewMigration
    but somehow I can not access the cake files
    i tryed;
    bin/cake
    cake
    cake migrations create MyNewMigration
    bin$ phinx create MyNewMigration

nothing seems to work
image`
image

I will further try it. For now just wanted to give an update on my workflow. And sry for spelling errors or style errors in messages or code. still learning XD

I think the code should be something like this:

`<?php
use Migrations\AbstractMigration;

class AddEnumValueToListVisibility extends AbstractMigration
{
public function change()
{
$table = $this->table('sentences_lists');

    $table->changeColumn('visibility', 'enum', [
        'after' => 'modified',
        'default' => 'unlisted',
        'null' => false,
        'values' =>[
            'private', 
            'unlisted', 
            'public', 
            'public+'] 
        ])->save();        
}    

}`

but I am not sure because i dont know how to test it XD

@AndiPersti did I miss something from your comments before? If yes sry, you have to remind me again

@AndiPersti
Copy link
Contributor

I decided for now that if the list is public+ and its editable by anyone that i still show the x.

Yes, that's a good catch because I've forgot about collaborative lists. 👍

But there's still another little issue: If a user is the creator of a list but the list is editable by no one, there should be no remove button.

So we have two cases when the button should be displayed:

  1. The list is editable by anyone
  2. The current user is the creator of the list and the list is editable by the creator.

Ok now we come to the main problem, the migration file. I am quite sure that I could write it from hand, but wanted to use
cake migrations create MyNewMigration
but somehow I can not access the cake files

It looks like you run the command from the Tatoeba/bin directory but you should call it from Tatoeba. If you run cd ~/Tatoeba && cake do you get the help text for the cake command?

But the code you've posted is a good starting point.

@bakananbanjin
Copy link
Contributor Author

It looks like you run the command from the Tatoeba/bin directory but you should call it from Tatoeba. If you run cd ~/Tatoeba && cake do you get the help text for the cake command?

I tryed it from every location, even updated migration
I also dont get the cake help page, dont know why
image

tried backslash slash..
https://book.cakephp.org/3/en/quickstart.html

anyway other then that I fixed this:

You don't need to add an additional parameter to find out where the request comes from. The request contains a Referer header with all the necessary information. In a controller the referer string is available through $this->referer().

and this

But there's still another little issue: If a user is the creator of a list but the list is editable by no one, there should be no remove button.

should close Tatoeba#895 and should close Tatoeba#1800

cakephp migration file still missing see comment
@AndiPersti
Copy link
Contributor

I also dont get the cake help page, dont know why

You get the error message

/usr/bin/env: ‘sh\r’: No such file or directory

so it looks like you somehow changed the line endings in bin/cake from the Unix \n to the Windows \r\n. (Have you ever opened that file from Windows and then saved it?)

I suppose running file /home/vagrant/Tatoeba/bin/cake will output /home/vagrant/Tatoeba/bin/cake: a /usr/bin/env sh script, ASCII text executable, with CRLF line terminators, doesn't it?

In order to fix this problem you should convert the line endings back:

cd /home/vagrant/Tatoeba
cp bin/cake{,.bak}
tr -d '\r' <bin/cake >temp && mv temp bin/cake

If everything works again, you can remove cake.bak (rm /home/vagrant/Tatoeba/bin/cake.bak).

@bakananbanjin
Copy link
Contributor Author

image

I think i will clone tatoeba imouto again, and try it one more time. XD
If it works I will load my changes again and everyone is happy. (maybe) sorry for the inconvenience

@AndiPersti
Copy link
Contributor

About the permission error: I forgot that you have to reset the executable bit on the file: chmod 755 bin/cake.

@bakananbanjin
Copy link
Contributor Author

OK why easy if I can make it difficult. after reinstalling imouto it works fine.
image

I hope with the last pull or push
The Issue @895 and @1800 is done XD

thx guys specialy thanks for all your help @AndiPersti

looking forward to hear your feedback

to see the delete button you should be at least logged in because public+ and anyone the delete button was still displayed
naming for public+ is still pending XD
@AndiPersti
Copy link
Contributor

ok I am a little stucked, how can I change the visibility condition, that it takes an OR value

SELECT * FROM sentences_lists WHERE visibility = "listed" OR visibility = "public";

As I understand it, I get a String in our case the word 'public' so I can not add an other condition for visibility in this construct?

You could just change the condition on line 217 in the model to use the IN operator and then pass the array ['listed', 'public'] in the controllers for the visibility parameter.

And this won't break calling getPaginatedLists with a string parameter, because whenever you use the IN operator in the where clause in combination with a string value instead of an array value like where(['visibility IN' => 'public']) CakePHP will do the right thing and construct the SQL so that it will look like visibility in ('public').

(A rather crude way to test this is adding the line

debug($this->SentencesLists->find()->where(['visibility' => 'public']));

to one of the actions in the SentencesListsController and load that page.

You should see in the debugging output the resulting SQL query.

Compare that with changing the debugging line to

debug($this->SentencesLists->find()->where(['visibility' => ['listed', 'public']]));

There is also a debugging plugin called DebugKit which would simplify debugging and e.g. would show you all SQL queries for a page but you need to install it in your local environment (and also modify some parts in the code; see the docs).)

@jiru
Copy link
Member

jiru commented Jun 22, 2020

sry somehow the code dosnt look that well

@bakananbanjin You can use triple backquotes to format code blocks.

see in commments for further info
@bakananbanjin
Copy link
Contributor Author

ok again an other update :)
sry was a little bussy the last days but still working on the issue.
as @AndiPersti mentioned we still have to do some rewording see his comments. Also I have to create some test for the code. (still pending xD)

other than that I tryed to fix all mentioned issues

@trang trang linked an issue Jun 30, 2020 that may be closed by this pull request
src/View/Helper/TagsHelper.php Outdated Show resolved Hide resolved
@jiru
Copy link
Member

jiru commented Jul 3, 2020

Hello 😊 I wondered what’s the status of this pull request, so I took a closer look and added a few (minor) comments.

@bakananbanjin Did you have time to think about a good wording for the tooltip? What about the tests you mentioned you wanted to write? Do you need some help from us, or you just need some more time?

@bakananbanjin
Copy link
Contributor Author

@jiru
thx for the update I changed everything according to your comments. the rewording I still wait for your reply.

AndiPersti 13 days ago Member

Maybe this tooltip also needs some rewording?
@bakananbanjin
bakananbanjin Author Member Pending

@AndiPersti maybe something like: The list is listed and can be searched.

?

And to be honest I thought the test case would be the next assignment after we finished this one (I looked a little bit in to test cases of tatoeba, but I got confused because it was just datasets).
I am also sry atm our corona restrictions got cut back, so I have to work on weekdays again, so I only have the weekends for tatoeba XD.

But i really would love to get this issue resolved soon! We spent now exactly 1 month on it, I was thinking this would be a job for a day or 2.

greetings

@AndiPersti
@trang
@bakananbanjin
@jiru

@alanfgh
Copy link
Contributor

alanfgh commented Jul 4, 2020

In general, when questions about wording come up, please add me (@alanfgh) to the discussion.

"The list can be found over Browse -> Browse by list, not at the sentence page"

This wording does need to be improved, but I'd like to understand the context better. Could you please explain when this tooltip would appear?

@AndiPersti
Copy link
Contributor

This wording does need to be improved, but I'd like to understand the context better. Could you please explain when this tooltip would appear?

This PR adds a new option to the sidebar on the list page where you can choose the visibility of the list. The tooltip is displayed for the old Public option which was renamed to Listed and should indicate that the list is only visible on the list page (and not on the sidebar of a sentence page; that's what the new Public option is used for):
tooltip

@AndiPersti
Copy link
Contributor

(I looked a little bit in to test cases of tatoeba, but I got confused because it was just datasets).

Do you mean the test fixtures inside tests/Fixture? These are used to setup the database for testing.

The code for the actual tests is inside tests/TestCase. This directory is structured like the src directory, i.e. you find for example the tests for src/Controller/SentencesController.php in tests/TestCase/Controller/SentencesControllerTest.php.

@bakananbanjin
Copy link
Contributor Author

image

thats new too. :(
other than that how is the issue going? still w8 for the right wording. other than that is the code ok?
Its weekend again and the weather is nice XD
and my back hurts www
sorry that was offtopic

@AndiPersti
@trang
@bakananbanjin
@jiru
@alanfgh

@AndiPersti
Copy link
Contributor

thats new too. :(

This is unrelated to your PR. See #2438.

other than that how is the issue going? still w8 for the right wording. other than that is the code ok?

All in all I think it looks ok.

Are you still trying to add some tests?

@trang
Copy link
Member

trang commented Jul 19, 2020

Here's my suggestion for the tooltips of each visibility option.

@alanfgh, do you confirm this is okay? (see @AndiPersti screenshot above for where these tooltips will be displayed)

Private
The list is accessible only to you.

Unlisted
The list is accessible by anyone but is not listed on the "Browse by list" page.

Listed
The list is accessible by anyone and is listed on the "Browse by list" page.

Public
The list is listed on the "Browse by list" page and is displayed on the sentence page.

@alanfgh
Copy link
Contributor

alanfgh commented Jul 19, 2020

Looks good. I would just standardize on "accessible to" rather than "accessible by" and maybe make the last option more explicit:

_Private
The list is accessible only to you.

Unlisted
The list is accessible to anyone but is not listed on the "Browse by list" page.

Listed
The list is accessible to anyone and is listed on the "Browse by list" page.

Public
The list is accessible to anyone and is listed on the "Browse by list" page, as well as on the sentence page for every sentence it contains._

But you might find the wording for the last item too long.

@jiru jiru added this to the 2020-08-02 milestone Jul 26, 2020
@jiru
Copy link
Member

jiru commented Jul 26, 2020

@bakananbanjin Good job with this PR, it is finally ready for merge 👍 I will publish it on tatoeba.org in one week so that UI translators have some time to translate the new strings. Thank you! 😊

@jiru jiru merged commit df83982 into Tatoeba:dev Jul 29, 2020
jiru added a commit that referenced this pull request Jul 29, 2020
@ckjpn
Copy link

ckjpn commented Jul 30, 2020

Public
The list is accessible to anyone and is listed on the "Browse by list" page, as well as on the sentence page for every sentence it contains._

But you might find the wording for the last item too long.

This is a bit shorter. Maybe seeing this will trigger an even more concise wording in someone's mind.

The list is accessible to anyone and is listed on the "Browse by list" page, as well as on each listed sentence's page.

@ckjpn
Copy link

ckjpn commented Jul 30, 2020

I was testing this on dev.tatoeba.org and noticed that the "Public (new)" doesn't actually get set.
I can click it, but when I return to the list, it reverts back to what it was.

Screen Shot 2020-07-30 at 13 19 40

@ckjpn
Copy link

ckjpn commented Jul 30, 2020

Just a quick follow up. I checked to make sure that it doesn't show on a non-CK account, and it doesn't.
(This account isn't the owner of the list.)

Screen Shot 2020-07-30 at 13 18 03

@AndiPersti
Copy link
Contributor

I was testing this on dev.tatoeba.org and noticed that the "Public (new)" doesn't actually get set.
I can click it, but when I return to the list, it reverts back to what it was.

Thanks for testing.
I've just pushed a fix and deployed it on the dev server. It should work now.

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.

Show list information on sentence pages for public lists Make it easier to remove a sentence from a list
6 participants