Apache Allura™

Updated with some unit tests

We should be thoughtful about the reactions list: 'thumbs_up', 'thumbs_down', 'laugh', 'hooray', 'confused', 'heart' Looks like the same ones GitHub offers. GitLab lets people pick any emoji at all. Should we consider more, or less? "laugh" surprised me, it is just a smile when GitHub renders it. I think we should call it "smile" since that's what the emoji is. And also "tada" instead of "hooray"? One idea would be to make it configurable for site administrators via a .ini setting. One of allura's strengths is its flexibility and customizability, so that might be a nice way to do it. Another thought would be to let every project customize it, but that seems like too much probably.

Maybe the counts should be a dictionary like react_users is, so that it can be more flexible if any are added or removed later. Would also make some methods like update_react_count a bit simpler.

Although VotableArtifact has a __mongometa__ too, neither get used since they are mixins. The data gets stored in the main class' collection, not vote or reaction. So probably good to remove those.

I'll test a bit more thoroughly after you look into those things too.

Hi..Dave.

I just hard coded current emoji list because this related merge request mentioned Github's reaction list. I also think customizablity via ini is good feature.

Appreaciate your ideas and made updates as per below

• User can override default emoji list by setting eg: reactions.emoji_list = :+1:, :-1:, :smile: in config ini. I just currently splitting value do we need to validate it ?
• changed persistant data structure as dict for counts
• made update_react_count by adding boolean parameter
• Update test cases (Currently I have added few cases I will check more situations and add if needed)
• removed counts and user lists when those gets 0 and [] respectively

Notices

I had to use utils.py to add a common method. moved js code from allura-base to widget since it is dynamic. updated app_globals with emojize method because it is required inside jinja templates

Can you advice regarding the changes

Thanks

Hey Shalitha,

This is great as far as functionality and configuration go. Mostly feedback on code setup / cleanup:

• JS inside of python files isn't a great pattern, even though there are many places in the code that do it. Can it be moved out into a regular .js file, so we promote better patterns? Classes and data- attributes can be a way to provide values that currently are dynamic inserted with %s.
• Also onclick isn't ideal, although allura code in other places does it too. Better to have regular .js file do some jquery with the class selectors.
• get_reaction_tooltip_content and other places use backticks aka template literals, but those aren't quite supported everywhere, so it'd be nice to not use them, so IE works still. https://caniuse.com/#feat=template-literals
• another option that could help is to output a global JS variable once from a master template and then reference it from the .js file: var reactions = {{h.escape_json(reactions)|safe}};
• in the tests, instead of assert ... and .. and .. can you split those into separate statements? That when when a test fails its clear exactly which part the problem is.

For the config setting, splitting it without validation is fine. If someone makes a mistake, its only site administrators and they can fix it :) What I would do though is put a reactions.emoji_list example in development.ini like we do for other settings so people can find it and see how it works.

If you wanted to add more functionality at this point, it could be nice to indicate which reaction you currently have. And be able to click on it to remove it.

Getting excited 😁

Hi.. Dave

Thanks for the feedback. Please check these modifications done...

• move js snippet to reactions.js in widgets and it will be linked via Thread widget
• added JQuery onclick instead onclick html event
• Removed backticks and replaced with single quotation
• Added global variable global_reactions via Thread widget and moved modified function to utils
• chunked logical assertions in to lines

Also emoji_unicode in json response is no longer needed since we already have list of emojis as a global var. Can we remove it from response. but there is rare situation when user reacts and at the same time admin changes emoji list

What do you think?

Hi.. Dave

I updated branch with modifications please check

👍

🥳