SpringAll
SpringAll copied to clipboard
wuyouzhuguli
Add a more detailed modal when reporting a post or a comment
As a moderator, I want to receive report only for posts and comments which violate the terms of use To easily focus on the important ones
Many reports received on pods I moderate are a people whining about others "look how bad they talk to me". As a consequence of that flow, important reports are sometimes lost in the middle of this. As an attempt to reduce "not real report", I implemented a real modal (instead of the previous javascript alert) which tell to the users to report only content which violates the terms of use, and remind them that they can block the other user if they want to. Also, the reason is required but not filled, which force them to fill something.
Here is how it currently looks like:
Many reports received on pods I moderate are a people whining about others "look how bad they talk to me".
If your intention of this PR is to address this issue, then it won't help. Mainly because those who file those reports also would not read the modal. If you want to get people to report only valid things, then you have to go the Facebook/Twitter route and have them select from a list of predefined reason, some of which end in a dialog explaining how to block people, while others actually do submit a report.
That's a good idea, but rise the question about how to make that generic for all pods... It means we could suggest a list the podmin would be able to edit, I guess.
I actually have the same question, but for your PR. Some pods, for example Geraspora, do not just refer to the ToS, but also to the project's community guidelines for content policing. Makes me wonder if we should expose that text in the config, but that would kill all chance of localizing that. Hm.
I rebased this PR, but I also marked it as Draft because of https://github.com/diaspora/diaspora/pull/8035#discussion_r296461071 if someone can explain this to me (@jhass maybe?)
@Flaburgan Can you check how this will show in mobile view? The Issue "Reporting in mobile" is yelled several times and I also think a mobile 'reporting' is very useful.
Un-requesting my own review here. I won't have the time to properly investigate this anytime soon, and I don't want to give the impression. <3
@Flaburgan Can you check how this will show in mobile view? The Issue "Reporting in mobile" is yelled several times and I also think a mobile 'reporting' is very useful.
The "report" feature doesn't exist at all in the mobile version, it would need to be added from scratch. I would say this work is out of scope of that PR and will be tracked in https://github.com/diaspora/diaspora/issues/8281
Un-requesting my own review here. I won't have the time to properly investigate this anytime soon, and I don't want to give the impression. <3
Sure no problem. Take care 😉
Okay I finally fixed my problem with the tests and I completed them, so this is ready for review again. @tclaus you probably want to rebase https://github.com/diaspora/diaspora/pull/8239 on it so you will have the report.feature file and will be able to complete it.
- The text-field is too high.
It was an <input> until now but Dennis made me change this to a <textarea>, that's why it is higher. It could actually be even higher than that to clearly show that several lines are okay, even if I feel that most reporters aren't going to use that. So I think it's good like it is now.
- When reporting another Item, the old reporting text appears. textfield should be cleared.
Yeah indeed. I saw that but I was wondering if that was a good point for people reporting several posts in a row, but I agree this is going the other way that what I am trying to achieve here, and can be disturbing. I'm going to fix that.
- Is there a default / placeholder text?
There is no default as we want to force the user to fill something. It could have a placeholder though, I'm going to add one.
- Can you set the focus on the input-field after opening the dialog?
Ok.
@SuperTux88 would you be able to tell me why the Jasmine tests are not running?
@SuperTux88 if you know why jasmine is not running I think this is the only problem here
@Flaburgan I have no idea why it's failing, it works perfectly locally. I rebased again on develop to remove the commits not belonging to this PR (so you want to "force pull" your local branch using git reset before you do any development again), and also added a commit to try to increase the timeout, however I have no idea if that helps. It would still be strange since it's usually finishing in under a minute, and if it suddenly takes more than two minutes, still something feels "broken" ... but lets see if it helps at all first, or if it's still timeouting. I think we added the timeout in the first place because when switching to GitHub actions we had problems with jasmine randomly getting stuck, but it's also strange that it now gets stuck with this PR consistently :man_shrugging:
Ok, it didn't help, I'm out of ideas then ... I removed my commit again :man_shrugging:
@Flaburgan If you change the chrome_cli_options you need to make sure that running it in docker still works (which needs the option currently on develop), so if GitHub-Actions need something special, then we need to set them conditionally (using an env-var for example). But I think the chrome_cli_options aren't the problem here, the options on develop work both in docker and on GitHub with the current develop code, so it's something in the code which breaks it, probably a change in app/assets/javascripts/app/views.js?
When looking at the log in github, the only one we have is [1001/011425.700187:ERROR:gpu_init.cc(453)] Passthrough is not supported, GL is disabled, ANGLE is. When you search for it on stackoverflow everyone is saying that Chrome Driver crashes after that error, so I think something is misconfigured...
The same error also is logged in develop where jasmine works fine, so I don't think that error is a problem (also if it would crash then it would just exit and not hang and timeout). I think it's more than one of the changes makes one of the tests run in an endless loop or something (no idea, just guessing), but it's strange that this only happens on GitHub. Maybe there is a way to give jasmine a more verbose output, to find where it's hanging?
Ok I'm not available this week-end so this will have to wait for next week.
@SuperTux88 looks like --trace isn't helpful on github. However, launching tests locally with jasmine:ci does hang on my computer. How could I know what's going on? Any way to display chrome logs in the console, or are they somewhere on my computer?
Actually it fails even when accessing locahost:8888 saying /getting_started/ isn't found 404
looks like
--traceisn't helpful on github.
--trace is a parameter for rake, not for jasmine, so it doesn't really affect jasmine.
However, launching tests locally with jasmine:ci does hang on my computer.
Then you are further than me, it works just normally on my computer and I can't reproduce these problems at all. If it also hangs locally then you should be able to test what helps locally much faster than pushing to github every time.
How could I know what's going on? Any way to display chrome logs in the console, or are they somewhere on my computer?
I just pushed another debug commit to print one line for each test (instead of printing just one long line with dots), to see if it at least executes a few of the tests or, if it just hangs from the beginning and doesn't even start
And re "Use JQuery instead of vanilla JS":
Vanilla JS should work as good (if not better) than jquery, and since this commit also cucumber fails, so I would just revert that again.
So yes, it looks like it's at least running a few tests, but then hangs. So probably not related to chrome errors at the beginning, since they are also printed when it's running and it also still runs a few tests now.
The last test which successfully runs is app.Router _initializeStreamView sets app.publisher, the next test that would run (when I run it locally it works) would be app.Router _initializeStreamView doesn't set app.publisher if already defined, so maybe that's the test which causes the hang? Maybe that helps you to debug that more.
@SuperTux88 I tried things but nothing work, if you want I'm available this evening to check together
@SuperTux88 ok so code wise we should be good to go. Jasmine is still hanging sometimes, I think @denschub investigated this and his suggestion was to remove the problematic test? I don't remember and I don't know where his commit is.
@Flaburgan I have some days off, so I can make a fresh rebase of #8239 to this one- (But no bigger code improvements)
I think @denschub investigated this and his suggestion was to remove the problematic test? I don't remember and I don't know where his commit is.
Yes, see #8360. The broken test is
https://github.com/diaspora/diaspora/blob/03796e8fe2421ddfbc5380b8194ee68afbe3b88b/spec/javascripts/app/views/bookmarklet_view_spec.js#L46
and it's re-enabled because you force-pushed my commit away. :D You can re-disable the test, simply by replacing the it( with a xit(. As I said in the linked bug, I think it's fine disabling this test for now - it's not critical, and we have a tracking bug open to eventually fix it.
t's re-enabled because you force-pushed my commit away. :D
Aaaaah sorry, indeed. I found back your commit https://github.com/diaspora/diaspora/commit/840478b2d2e5bd2162929b033426a18201c27b4b but even after fetching your fork I was not able to cherry-pick it. That's fine I just rewrote it. So, CI should be green soon, fingers crossed.
Do we want to disable the display of Jasmine output that we are introducing here or do we want to keep it?
Do we want to disable the display of Jasmine output that we are introducing here or do we want to keep it?
That commit was only for debugging and should be removed, same for the increase of the timeout (Both were already removed, but you brought them back when you removed the commit that disabled the test).