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

Add a "No Access" badge to subscribers that will not be able to receive notifications #461

Merged
merged 4 commits into from
Dec 19, 2018

Conversation

WPprodigy
Copy link
Contributor

For #451

Recap of what's being done in this PR:

  1. Adds a "No Access" badge for users on page load. The permissions checks are only done for users that are already subscribed.
  2. When users are selected and saved via ajax, the response sends back a list of checked user IDs that do not have access. Can then use this list to find out if the newly selected user needs the badge or not.

I prototyped a bit ahead, thinking of how #273 could be implemented. So some of the organization here had those changes in mind.

Also note that I added the user_can_be_notified() method in for testing purposes, but can remove/leave out when it comes to merging into the feature branch.

Screenshot:
no-access

@jerclarke
Copy link
Contributor

Hi all, I'm not sure of the best way for me to fix the conflicting files problem. If someone could tell me the basic steps I need to take to submit an update to something like this that would be appreciated (i.e. starting from my own forked master, what do I do?)

I got it working on my local setup, alongside #449 and current master branch (i.e. #452) but there were several weird merge things.

I'm a bit out of my depth with the git here, trying to test all these things together, sorry.

The main, non-merge-related problem is that this totally breaks the display of #452 because this changes the HTML structure of the checkbox area, on which #452 depends.

This jQuery replacement in editorial-comments.php fixes the display of people who will be notified:

 		subscribed_users.each( function() {
-			usernames.push( $( this ).next().text() );
+			usernames.push( $( this ).parent().siblings('.ef-user_displayname').text() );
 		} );

The earlier one was a bit brittle overall in retrospect. Maybe someone else can come up with a more robust version of my jQuery, but you get the idea.

Without this, merging master into feature/451-notification-visibility results in the recipient display becoming just a series of commas, with no errors.

Going to try to submit this as a PR somehow.

@jerclarke
Copy link
Contributor

@rinatkhaziev Please see PR above, it's vital to get it in before any release, as this whole feature is currently broken without the fix.

@GaryJones GaryJones deleted the feature/451-notification-visibility branch December 9, 2019 15:01
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.

None yet

3 participants