Issue 643 #770

Martii · 2015-10-17T04:49:03Z

Read specific commit notes since this was at least a ~~two~~ six part'er.

Applies to #643 and leads into #641

Tested on dev and local pro

A screenshot, perhaps more:

* Base working concept * Eventual showing of the reason in respective views... currently set to a horizontal ellipsis if absent... see additional notes below Applies to OpenUserJS#643 and OpenUserJS#641 NOTES: * Test with QSP `?flagged=true` for lists and Admin or better privileges on script homepage and user homepage with logged in and logged out... there are at least 6 routes *(4 with QSP and 2 without but all elevated to Admin+)* needed to test against * If for some reason some user is flagged but there aren't any actual flags it will show up as no user and no reason e.g. Flagged column will be empty for those mustache opts * Using series in some of the *async* related calls to hopefully ensure natural ordered flags * **NOT COMPACTED** for clarity as well as for strict *async* usage * One bug fix in a view with colspan * One restoration in `.../modelParser.js` * Some class changes in a view TODO: *(mitigation)* * Needs reason attached to flagging route ... separate issue to be created and milestoned near OpenUserJS#485 and OpenUserJS#262 and similarly done in OpenUserJS#513

* Standardize on `flagList` since `flagged` is already in use * Some compaction of the code ... backed out a few changes in prior commit... added a master asynchronous function to the flag library * One bug fix with `modelParser` to acccomodate prior commit reversion **NOTES**: * This should be functionally equivalent to the last commit feature wise and a lot easier to maintain. An example of this is the dynamic field name change in this commit... easily changeable in case someone doesn't like that name but should be okay for now. Applies to OpenUserJS#643 and indirectly OpenUserJS#641

Martii · 2015-10-17T04:49:54Z

@sizzlemctwizzle
bump

Martii · 2015-10-17T07:45:07Z

Misc notes (before merging... questions may change):

~~Consider renaming setFlaggedListInModel to parseFlag...IntoModel and move into .../modelParser.js~~ ? e.g. ~~is there a better name~~ ~~and a better location~~?
Include all model requires where ever this lands (distribution in the project tree) so if someone uses 'Comment' down the line it won't bomb since right now only 'User', 'Flag' and 'Script' models are in use ? (may be moot here)
~~More STYLEGUIDE.md conformance in affected functions ? *(mainly newlines I think with perhaps some forward to ES6 compliance~~ ... ./controllers/user.js is huge so needs lots more testing)* ... (perhaps do later but constantly)
...

Martii · 2015-10-17T09:26:21Z

controllers/user.js

+    if (aErr) {
+      aNext();
+      return;
+    }


MITIGATION: Test why this error fall through is the only one (for express)... I am guessing if we (ever) have an error in the changed views ("elsewhere") we're going to 500 and not fail gracefully.

asyncComplete should always test for an error and handle it (at the very least a 500 page). If we can catch an error earlier on then great, but this is really the last line of defense. It would be nice if we could find a way to abstract this function and put it in libs/helpers.js since I hate seeing the same logic repeated, but I realize the difficulty of doing this.

Kind of figured that with the missing aNext() and it raised an eyebrow yesterday in my background thinking... I think I'll do that in a single PR with some cleanup just on ./controllers/user.js... I read elsewhere that ES6 is going to need undefined for certain functions instead of null (or worse some other huge object in a bug) so that is why I have been splitting out the aNext() and return statements... also it makes more sense not to return something that isn't there and could change down the line.

Just a misc note too node 4.x went LTS (Long Term Support) according to their site e.g. 5.x is around some corner.

Martii · 2015-10-17T23:45:33Z

Need some input from those questions above before anyone can merge.

Additional note...we could eventually show Mods just the reason, but not the user/author, so they can make an informed decision as to flag a critically flagged script/user but I left that alone for this PR.

sizzlemctwizzle · 2015-10-18T04:09:11Z

libs/flag.js

@@ -148,3 +150,51 @@ exports.unflag = function (aModel, aContent, aUser, aCallback) {
    }
  });
 };
+
+exports.setFlaggedListInModel = function (aModelName, aOptions, aCallback) {


This is definitely the wrong module for this function. This module is supposed to be completely devoid of any front-end view logic (notice how "aOptions" or "options" are mentioned nowhere else in this file). This module defines the logic for how flagging is handled.

So where should it go?... I don't have a clue since this function is a "one of a kind new" thing AFAIK

Answered in my comment below.

sizzlemctwizzle · 2015-10-18T04:20:24Z

Consider renaming

Yes, as my line comment says, the code should be moved. Create a flag.js controller. Remove has a model, lib, and controller. Flag and vote should be structured like this as well. So you might as well start the flag controller for this.

Include all model requires where ever this lands

There is code for this already. Vote, Flag, and Remove are all actions that I implemented in that order. They are very similar so the implementations got more well thought out as I went (with Remove being the best version). I suspect that's why you're having a problem finding the right place to put the new flag function, since it's controller logic and Flag doesn't currently have a controller.

More STYLEGUIDE.md conformance in affected functions

Nice to have, but lower priority.

Martii · 2015-10-18T04:40:44Z

Create a flag.js controller.

Done locally... need to link it in though.

Remove has a model, lib, and controller.

That appears Greek to me at the moment... what?

Flag and vote should be structured like this as well.

Did you want me to create a base vote.js controller too?

There is code for this already. Vote, Flag, and Remove are all actions that I implemented in that order. They are very similar so the implementations got more well thought out as I went (with Remove being the best version). I suspect that's why you're having a problem finding the right place to put the new flag function, since it's controller logic and Flag doesn't currently have a controller.

I'll have to wait and see what you say on this part once I create the controller(s).

Thanks for the feedback... more to tinker with to get this reworking. Will buzz you later.

* Created `flag.js` controller as per sizzle and moved the export there * Created `vote.js` controller base * Move/remove last commit stuff out that was added in. * Renamed `setFlaggedListInModel` to `setFlaggedListToModel` ... this still could use some advice on exact name that is wanted for the identifier. * NOTE'd a possible extra dir traversal that shouldn't be needed as compared to below requires... this is beyond this PR and will retest/cleanup in another unrelated commit so just noting it Applies to OpenUserJS#643

Martii · 2015-10-18T05:26:34Z

@sizzlemctwizzle
Buzz, buzz, buzz! :) Now if you want me to rename the function name please give me an exact identifier please otherwise I'll just go with that function name. If I were to include the base controller in as flag we might have an issue as that identifier could be taken elsewhere... so I just imported the jsm function itself.

I still don't have a clue what you meant by "Remove has a model, lib, and controller.".

Did create a vote.js controller just as a placeholder to avoid future confusion perhaps. Not linked in anywhere though.

Noticed a weird require too and notated it... probably why it has newlines around it... seems like something I would have done early on but not sure.

Now what is the correct next step? Fully retested on dev at current PR tip since this is super fresh and that was a simple move.

sizzlemctwizzle · 2015-10-18T05:41:09Z

I still don't have a clue what you meant by "Remove has a model, lib, and controller.".

Remove action:
Model: models/remove.js
Lib: libs/remove.js
Controller: controllers/remove.js

Flag action:
Model: models/flag.js
Lib: libs/flag.js
Controller: all flag controller logic is currently spread out amongst multiple controllers, but I'd like you to create a controllers/flag.js file and put your function in it (the current name is fine)

Vote action:
Model: models/vote.js
Lib & Controller: all logic for voting embedded and spread out amongst multiple controllers, but you don't need to create any files until you have something to put in them

Martii · 2015-10-18T05:42:31Z

Oh durrr... I was taking "Remove" as something you wanted me to do... not the controller itself. Sawwy.

Martii · 2015-10-18T05:44:16Z

So did you want a different identifier name for this function e.g. signature or do you think this is PR READY?

sizzlemctwizzle · 2015-10-18T05:48:54Z

How about getFlaggedListForContent?

Martii · 2015-10-18T05:51:11Z

but you don't need to create any files until you have something to put in them

Disagree due to the confusion. If I come back to a similar case where I can't figure out where to put it I may not recall this conversation.

getFlaggedListForContent

I'm game for that... I thought about calling it sizzle since I had no clue on naming conventions and especially now since it's in a new controller.... k uno momento for the identifier change. I tossed back and forth between Model and Content during this anyhow... just didn't know what was expected. Thanks.

sizzlemctwizzle · 2015-10-18T05:57:02Z

If I come back to a similar case where I can't figure out where to put it I may not recall this conversation.

Good point. I actually found myself unable to remember which actions lacked which files.

I tossed back and forth between Model and Content during this anyhow

If I see Model in the name it makes me think I'm going to get (mongoose) model objects back from the function, when this function really adds more content via aOptions.

* Using this to be more consistent with other contributors and upon the advice of Lord Sizzle! :) Applies to OpenUserJS#643

Martii · 2015-10-18T06:11:40Z

Okay... done and retested (logged out, logged in with anyone, logged in with Admin) ... anything else before you test it with PR READY?

I've done a shiz load of testing on this function (over 50 hours) especially when I compacted it so it should be good to go. Thanks for your patience with me and "hand holding" at my request... I'm still not clear on MVC and all the local book stores have nothing on this style of coding... search results just aren't doing it... may have to pay a visit to a University to get a hold of a book! :)

* As described earlier placeholder so anyone knows where to start to put things without a search NOTE: Specific to OpenUserJS#770 (comment) Applies indirectly to OpenUserJS#643

* This is somewhat flexible when needed but will assist in directing contributions. This is mostly the current pattern. * During a cleanup phase will probably match this general code styling in the other controllers Applies indirectly to OpenUserJS#643

Martii · 2015-10-18T19:14:04Z

Auto-merging soon as retests and pouring over the code for a few more hours seems good to go.

Issue 643 Auto-merge

Martii added 2 commits October 15, 2015 16:37

Martii reviewed Oct 17, 2015
View reviewed changes

Martii added the question A question has been encountered by anyone and has remained unanswered until cleared. label Oct 17, 2015

sizzlemctwizzle reviewed Oct 18, 2015
View reviewed changes

Martii removed the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Oct 18, 2015

Change the identifier for the function name

01cb33e

* Using this to be more consistent with other contributors and upon the advice of Lord Sizzle! :) Applies to OpenUserJS#643

Martii added needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. and removed question A question has been encountered by anyone and has remained unanswered until cleared. labels Oct 18, 2015

Create/fill in the missing lib for vote action

ad2edb1

* As described earlier placeholder so anyone knows where to start to put things without a search NOTE: Specific to OpenUserJS#770 (comment) Applies indirectly to OpenUserJS#643

Martii added the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Oct 18, 2015

Martii removed the needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. label Oct 18, 2015

Martii added a commit that referenced this pull request Oct 18, 2015

Merge pull request #770 from Martii/Issue-643

89e2321

Issue 643 Auto-merge

Martii merged commit 89e2321 into OpenUserJS:master Oct 18, 2015

Martii deleted the Issue-643 branch October 18, 2015 19:28

Martii removed the PR READY This is used to indicate that a pull request (PR) is ready for evaluation. label Oct 18, 2015

Martii mentioned this pull request Oct 23, 2015

Modify view and controller to allow setting of a fixed reason when flagging #775

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 643 #770

Issue 643 #770

Martii commented Oct 17, 2015

Martii commented Oct 17, 2015

Martii commented Oct 17, 2015

Martii Oct 17, 2015

sizzlemctwizzle Oct 18, 2015

Martii Oct 18, 2015

Martii commented Oct 17, 2015

sizzlemctwizzle Oct 18, 2015

Martii Oct 18, 2015

sizzlemctwizzle Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015

Issue 643 #770

Issue 643 #770

Conversation

Martii commented Oct 17, 2015

Martii commented Oct 17, 2015

Martii commented Oct 17, 2015

Martii Oct 17, 2015

Choose a reason for hiding this comment

sizzlemctwizzle Oct 18, 2015

Choose a reason for hiding this comment

Martii Oct 18, 2015

Choose a reason for hiding this comment

Martii commented Oct 17, 2015

sizzlemctwizzle Oct 18, 2015

Choose a reason for hiding this comment

Martii Oct 18, 2015

Choose a reason for hiding this comment

sizzlemctwizzle Oct 18, 2015

Choose a reason for hiding this comment

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

sizzlemctwizzle commented Oct 18, 2015

Martii commented Oct 18, 2015

Martii commented Oct 18, 2015