[WIP] Feature: Warning system for #47 #268

Merged
merged 11 commits into from Nov 26, 2016

Projects

None yet

4 participants

@Matslom
Member
Matslom commented Nov 18, 2016 edited

#47

  • Need some js
  • Make better design
  • Warn a user
  • Warn a user for a particular piece of content (post, topic etc)
  • Configurable presets for warning points and reasons
  • Warn using a preset, or custom reason and points
  • Revoke warn
  • Acknowledge with warn
@euantorano

Just a couple of minor nitpicks, otherwise this looks good!

app/Database/Models/Warning.php
+ */
+ public function getPresenterClass()
+ {
+ return 'MyBB\Core\Presenters\WarningsPresenter';
@euantorano
euantorano Nov 18, 2016 Member

Can you please use the ::class syntax here - \MyBB\Core\Presenters\WarningsPresenter::class?

This allows IDEs to change the classes automatically if we rename them for any reason.

app/Database/Models/Warning.php
+ */
+ public function issuedBy()
+ {
+ return $this->belongsTo('MyBB\\Core\\Database\\Models\\User', 'user_id');
@euantorano
euantorano Nov 18, 2016 Member

Again, please use \MyBB\Core\Database\Models\User::class here.

app/Database/Models/Warning.php
+ */
+ public function revokedBy()
+ {
+ return $this->belongsTo('MyBB\\Core\\Database\\Models\\User', 'revoked_by');
@euantorano
euantorano Nov 18, 2016 Member

And again, please use ::class ๐Ÿ˜„

+ */
+ protected $warningTypeModel;
+
+
@euantorano
euantorano Nov 18, 2016 Member

Unnecessary blank line - no big deal but may as well fix it now :)

+
+ return $warningType;
+ }
+
@euantorano
euantorano Nov 18, 2016 Member

And another empty line that could be removed.

+ */
+ public function findForUser($userId)
+ {
+ return $this->warningModel->where('user_id', $userId)->with(['issuedBy'])->orderBy('created_at', 'desc')->get()->groupBy('expired');
@euantorano
euantorano Nov 18, 2016 Member

Could this be broken across a couple of lines please, just to make it easier to read?

+ {
+ return $this->warningModel->find($warnId);
+ }
+
@euantorano
euantorano Nov 18, 2016 Member

And another empty line ๐Ÿ˜„

+ *
+ * @return mixed
+ */
+ //public function allForUser($user_id);
@euantorano
euantorano Nov 18, 2016 Member

Is this function going to be implemented, or can it be removed from the interface?

+ 'must_acknowledge' => $request->input('must_acknowledge'),
+ ]);
+
+ return redirect()->route('admin.warnings.warning_types')->withSuccess(trans('admin::warnings.warning_type_create_success'));
@euantorano
euantorano Nov 18, 2016 Member

Could this be split across multiple lines too please?

+ 'must_acknowledge' => $request->input('must_acknowledge'),
+ ]);
+
+ return redirect()->route('admin.warnings.warning_types')->withSuccess(trans('admin::warnings.warning_type_edit_success'));
@euantorano
euantorano Nov 18, 2016 Member

And another long line.

+
+ $warningTypes = $this->warningTypesRepository->all();
+
+ return view('warnings.warn_user', compact('user', 'contentType', 'contentId', 'warningTypes', 'previewContent'));
@euantorano
euantorano Nov 18, 2016 Member

Another long line that could be split across multiple lines please.

@euantorano
Member

Also some code style changes needed it seems.

Thanks for the hard work so far though - this looks really well done!

@Matslom Matslom changed the title from [WIP] Warning system to [WIP] Feature: Warning system for #47 Nov 18, 2016
@euantorano
Member

Looks good to me - I remember you saying you weren't too good at JS, so do you want to leave that and either I can add it later or somebody else can?

@Matslom
Member
Matslom commented Nov 19, 2016

My js is very messy + I did not do anything with typescript previous. It will be nice if someone can do this.

@euantorano
Member

No problem, I can do that :)

On 19 Nov 2016, at 20:17, Matslom notifications@github.com wrote:

My js is very messy + I did not do anything with typescript previous. It will be nice if someone can do this.

โ€”
You are receiving this because you commented.
Reply to this email directly, view it on GitHub #268 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAttcYNBSK_d2F53LOjvjG2w4Fokmwkeks5q_1lNgaJpZM4K2rkj.

@premiumcuyuz

good work :) @euantorano @Matslom We are waiting for more ๐Ÿ‘

@036
036 commented Nov 21, 2016

Looks good @Matslom good job.

@euantorano
Member

Looking very good, thanks!

@euantorano euantorano merged commit 633e228 into mybb:master Nov 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Matslom Matslom deleted the Matslom:warning-system branch Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment