I am a drupal developer and I have been developing drupal modules for 3 years. I been looking for a long time module who give me a opportunity render a view field with user status and i have never found so I wrote that kind of module.

Module provides View field to views and Pseudo field to display user online status

This module provide a status based on strategy pattern and default display three different strategy:
if user is online then display: online status
if user is offline then display: offline status
if user is only absent then display: absent status

You can easily add new own strategy created a new php class
All functionality is based on oop pattern so you can easily use that code somewhere in your module.

Project link

https://www.drupal.org/project/user_status_online

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/user_status_online.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-user_status_on...

Comments

crafter created an issue. See original summary.

crafter’s picture

Issue summary: View changes
apaderno’s picture

Issue summary: View changes

Thank you for applying! I added the Git instructions for non-maintainer users and the PAReview checklist link. Reviewers will check the project and post comments to list what should be changed.

If you haven't done it, yet, please check the PAReview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

crafter’s picture

Ok I have already fixed the information in my README.MD

apaderno’s picture

Vernit’s picture

There are so many issue reported by PARreview specially "When importing a class with "use", do not include a leading \". Please resolved all of them.

crafter’s picture

Thanks for info I fixed all that issues and many others.

crafter’s picture

Status: Needs review » Fixed
apaderno’s picture

Status: Fixed » Needs review

Until the vetted role isn't given, nothing has been fixed.

Dakwamine’s picture

Status: Needs review » Needs work

Hello crafter,

Code standards

Can you add doc comments on your functions and comply with Drupal's code writing standard, as suggested by the review tool?

https://pareview.sh/pareview/https-git.drupal.org-project-user_status_on...

Using the same code standard makes contribution easier for everyone and improves readability.

Redundant module

On another note, there is some similarity with the user_online_status module, as stated in the issue queue: #3099978: Difference with user_online_status. I understand the differences in the design of the code, and the added features (the view field) makes a nice enhancement of the original idea, but was it not possible to iterate on the original module by asking the maintainer access to the repository to prepare a new version (8.x-2.x) instead of creating a new module? This is to prevent code duplication, as we can see in the .module file.

If this is really not possible, please state the differences on the module's home page.

Other considerations

Does this module handles cache invalidation properly? Have you considered use cases where your module can be used? If so, it may be interesting for the end user to state what should not be done in a documentation page.

crafter’s picture

Ok, Dakwamine maybe you're right, thank you.

crafter’s picture

Can you add doc comments on your functions and comply with Drupal's code writing standard, as suggested by the review tool?

I don't know what it's wrong there so I can't fix that. If you have any ideas could you give some advise about that.

Best Regards.

apaderno’s picture

The previous comment is referring to the documentation comments posted in the code before class properties, methods, or functions. The Drupal coding standards say what those comments should report, in API documentation and comment standards.

For example, the documentation comments in the following code are incomplete.

  /**
   * @param \Drupal\user_status_online\StatusStrategy\StatusStrategy $strategy
   *
   * @return mixed
   */
  public function addStrategy(StatusStrategy $strategy);

  /**
   * @return string
   */
  public function getStatus();

  /**
   * @return \Drupal\user\UserInterface
   */
  public function getUser();

  /**
   * @return \Symfony\Component\HttpFoundation\RequestStack
   */
  public function getRequest();
crafter’s picture

ok thank you I will try fixed that later.

crafter’s picture

Ok I added doc comments on my function.
I added also information about differences on the module's home page.

crafter’s picture

Status: Needs work » Needs review
batkor’s picture

Status: Needs review » Needs work

What is it?
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/user_...
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...

Please corrective code.
Documentation
https://www.drupal.org/docs/develop/standards/coding-standards#indenting
https://www.drupal.org/docs/develop/standards/object-oriented-code

Use service in https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/user_...

$variables['status'] = t(\Drupal::service('user_status_online.service')->getStatus($user));

Nitpick
Return object in function https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...

public function addStrategy(StatusStrategy $strategy) {
    if (in_array($strategy, $this->strategies, TRUE)) {
      return $this;
    }
    array_push($this->strategies, $strategy);
    return $this;
  }

and change code

$statusManager->addStrategy(new OnlineStrategy())
->addStrategy(new OfflineStrategy())
->addStrategy(new AbsentStrategy());

Check User object https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/P...
$user instanceof User

crafter’s picture

Thank you for your comment and code review I have just fixed all.

crafter’s picture

Status: Needs work » Needs review
batkor’s picture

Nitpick
I didn't check
THis method
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
change

  if (!in_array($strategy, $this->strategies, TRUE)) {
      array_push($this->strategies, $strategy);
    }
    
    return $this;

Remove this comment
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
And change comment option
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
to

/**
   * List all strategies
   *
   * @var \Drupal\user_status_online\StatusStrategy\StatusStrategy[]
   *   Array of all status strategies.
   */
  private $strategies = [];

Update Rareview.
Please fix problems
https://pareview.sh/pareview/https-git.drupal.org-project-user_status_on...

Change comment for method in object interface
https://git.drupalcode.org/project/user_status_online/blob/8.x-1.x/src/S...
to
* @return self or * @return \Drupal\user_status_online\StatusInterface

batkor’s picture

Status: Needs review » Needs work
crafter’s picture

Status: Needs work » Needs review
dqd’s picture

@crafter: Thanks for the contribution.

Sorry for coming back to the basics on the end of the issue here. But I really would like to suggest and discuss a name change for the project. The amount of badly (double) named projects has been raised. And since this project here differs from the other one regarding the user status views field implementation (from what I understood) and since it still seem to confuse with the other module like #10 correctly states, we should consider to ask the project contributor to change the projects name to better seperate it from the other project. Maybe something like "Views field user online status"? Or something better? I see the problem and this project here tries to make things "better" than its predecessor, but users are confused about 2,3 projects having almost the same name but in a different order.

BTW: Does the worries of #10 have been discussed completely and has the difference been stated on the project page? EDIT: ok it has been addressed on the project page, but I would strongly recommend to clean up the project info page, which has a lot of doubled phrases and sentences and needs some basic and clear structure.

Just some nitpicks from my side. Happy New Year @ all.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for you contribution!

  1. LoadTest: looks like the test does not actually test a view and check if the user status is correctly displayed? What is the point of this test?
  2. AbsentStrategy looks wrong. Shouldn't this return TRUE if the user never accessed the site? Please explain in a code comment what "Absent" means.

Otherwise looks good to me, did not see any security issues.

crafter’s picture

ok thank you for your comments. I going to change this module a little bit. I am going to change module name i will fix all that issues soon.
In the meantime I close that ticket and I am going to reopen that again when I will be ready. @klausi Could you give me some advice about absent strategy and how it would look like according to you?
Thank you for your help.

crafter’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
crafter’s picture

Status: Closed (duplicate) » Closed (outdated)
apaderno’s picture

Status: Closed (outdated) » Reviewed & tested by the community
Issue tags: -PAreview: project created less than ten days ago

The application has been reviewed and considered good to go. There isn't anything else that needs to be done: The application just needs to be approved.

apaderno’s picture

Assigned: Unassigned » apaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

crafter’s picture

Thank you @kiamlaluno and I appreciate that.
It was very helpful for me because it was my first contribute module.
I going to contribute drupal and learn more about that process and join to group.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

leymannx’s picture

user_online_status maintainer here. Found this module by accident and already commented on https://www.drupal.org/project/user_status_online/issues/3099978#comment.... OP did never contact me and there's no mention of the already existing module. Furthermore user_status_online actually introduces quite a major difference to mention: The online status gets added via markup and cache tag max age 0. As far as I understand it this then bubbles up and disabled cache for the complete current page. user_online_status on the other hand prevents that in the first place by providing an uncached endpoint and calling it via Ajax to place the online status on the page.

Yes, user_online_status has quite some todos open, which are on my agenda for the global contribution weekend. Nevertheless I find it quite not right how things went here.

crafter’s picture

Hey and thank you for your comment. I think that user_online_status is not reusable module and not so I can't use that module in another module because i would have to copy a lot of code and paste all case condition. Maybe you are right I should at first contact with you and tell you to add needed functionality to module but I didn't have time for that because I needed that then. I think that it's a lot of similar modules here because it's contribute area so everyone wants to add something nice. If you see a problem with that ok, no problem I can add in description that module is based on your module or sth like that. If you offended in any way so I am very sorry, but I didn't want to.

leymannx’s picture

Hm, "sorry not sorry" is what I read from your answer. And I'm also wondering why you say the module wasn't reusable. Therefore you copied quite a bit of it. The theme hook, the pseudo field the template. What about about label and label_display?

And are you even aware of the caching issue? Why you disable the whole page cache? Have you tried to add the user ID as cache tag? Did it work?

And what about the test you added to the module? You set up a Drupal, create an admin user and then check if the front page is working. Why don't you test the user page? Or test if your service is working?

Honestly I wish your status would be revoked for the course of action you set here.

crafter’s picture

o my goodness man take it easy a little bit. I don't want to argue with you.

leymannx’s picture

o my goodness man take it easy a little bit. I don't want to argue with you.
If you think that your module is reusable please read about SOLID or DRY
etc.. and look back at your module again maybe then you understand. And
please don't tell me about revoked etc because it's not your business. I
think that people like you destroy all contribute idea.

No need to be unfriendly or unkind. Contribution means to help others to make an existing module better. Unfortunately you skipped that chance and chose to launch a duplicate module instead. And on that duplicate module's project page you chose to describe the already existing module as inferior.

I'd say there are quite some things that need to be discussed here.

crafter’s picture

I am new here so I didn't know that it's will be any problem with that. If I'd known that I would never make this module. I don't need that module to be here because it not my case. I thought it will be ok and people will be happy if i contribute to develop drupal module. But I see that it is only problem so if you have a permission please revoke my module and give me break man.

See you

leymannx’s picture

I am new here so I didn't know that it's will be any problem with that. If I'd known that I would never make this module. I don't need that module to be here because it not my case. I thought it will be ok and people will be happy if i contribute to develop drupal module. But I see that it is only problem so if you have a permission please revoke my module and give me break man.

Why are you saying you didn't know this was a problem? Multiple reviewers in this thread raised their concerns about the already existing module.

@kiamlaluno – This issue should be reopened, status set to "Needs work" to address the following issues, and until they are fixed the module should be opted out of security advisory coverage and the permission to opt-in should be revoked from OP.

1. "User Status Online" (USO) module's project description must provide a fair comparison explaining endusers notable differences to the ajaxified approach of the original "User Online Status" (oUOS) module.

USO is cache tagging the user entity extra field markup with max age 0 to bubble up and disable the whole current page cache. The user entity extra field also may not always display up-to-date information with server-side caching strategies/proxies in place. Whereas on the other hand oUOS requires JavaScript and fetches the information via Ajax to always display up-to-date information, only one single route may need to be excluded from server-side-caching.

(I'm not sure how the USO's Views field is behaving though, maybe Views takes care of cache tagging the Views field with the user entity ID and automatically recognizes differences in the user's online status like Drupal's built-in "Who's online" view does it?! Maybe @crafter can clarify.)

2. Any text indicating the oUOS module or Ajax/JavaScript as inferior must be removed from the USO project description.

3. USO should provide documentation how {{ label }} and {{ label_display }} in the user entity extra field template are supposed to be used.

4. USO tests should be re-reviewed and maybe need to be rewritten to test the actual online status service or the existence of an online status string on the user page instead of just testing status 200 on the front page.

klausi’s picture

@leymannx: I certainly understand your frustration. In general we try to encourage contributors to not duplicate modules, but this has been removed from the security coverage application process requirements. Now the main focus is security review.

Contributors are allowed to duplicate modules if they think their approach works better for them, or whatever they consider advantages. I don't see any insults on the project page, just a couple of sentences how the module is different. Also no mention of what module is inferior or superior. Maybe that has been cleared up already by crafter after your feedback? I would think that is a positive outcome on your comments. We should be patient with new contributors and they should be respectful to existing work by other contributors.

So from our point of view this issue is resolved and @crafter will keep his security coverage approval.

gisle’s picture

Just to add my two cents worth. We still need to discourage duplications in the PA process.

While duplication may sometimes be tolerated as the last resort when the current project owner ignores or rejects patches and one feels that those are necessary* for the project to be usable for one's projects, it still need to be part of the PA process. We cannot have reviewers that simply igores it.

In comment #32 leymannz writes:

OP did never contact me and there's no mention of the already existing module.

While the requirement to not duplicate projects has been removed from the security coverage application process for some time, I still think PA reviewers should still require:

  1. That the author of the duplicate project is first approached and cooperation discussed.
  2. If the suggested cooperation does not pan out, that the duplicate project is described on the project page with the differences pointed out.

Those minimal requirements are not fulfilled here, and I therefore think it was wrong to approve this application before the applicant sorted this out.

The guidelines used to contain a section saying "Ensure your project is not a duplication" - see https://www.drupal.org/node/1587704/revisions/9866535/view . This section was removed in entirety by klausi on 9 Mar 2017 at 22:23 CET. I think this was very unfortunate. IMHO, a softening of the language would have been a more appropriate measure. I would like to see this section restored in some form, as needless duplication is indeed poisonous to our ecosystem. The PA review is about the only place we can teach newcomers good citizenship.

*) For the record: I've duplicated a project myself. I did this when creating Phone Field, which is an obvious duplication of Telephone. I did this out of frustration with this issue: #2109227: Feeds Import Integration for nodes being ignored. (I sent a PM to the the owner about cooperation, but never received a reply.)

leymannx’s picture

@klausi – There's the following section on the project page.

Module is similar to
user_online_status

but has lots of differences

First at all my module provide a new view field plugin which display status as a view field.
Pseudo field was rewritten. I write custom template preprocess and not need an ajax and js staff.

Additionally I wrote that module using oop strategy pattern, code is reusable.

In contrast to the original module OP describes their module doesn't need "an ajax and js staff". As if Ajax or JS would be something bad?!

In contrast to the original module OP describes their module is "using oop strategy pattern, code is reusable". As if the original module's code is not?!

OP answers to my comments in this thread here also are not the nicest. I feel offended TBH. Your answer to that feels unbalanced.

Additionally me too would wish the duplication clause would not have been removed from the requirements. Where else do you suggest then I should file my concerns regarding duplication or the offense?

klausi’s picture

@leymannx: publishing a similar module with a different approach is allowed on drupal.org, so there is no offense. I consider the language on the project page fairly neutral and not insulting, so I also don't see an offense there.

That leaves us with the issue queue of the project where you can file an issue for improvements of the project page or an offer for collaboration. Since the conversation was quite hostile in this thread already please do that in a constructive and friendly fashion.

@gisle: the project application process was removed completely in 2017 because we could not keep up with applications, see https://www.drupal.org/drupalorg/blog/goodbye-project-applications-hello... . We replaced it with the security advisory coverage process. I think some documentation pages are not fully up to date yet. We agreed to repurpose the project application queue to be a queue for vetting users for the ability to opt their projects in to receive security advisory coverage. For that we only have to review their ability to write secure code.

leymannx’s picture

Glad you don't see an offense. Problem solved.

leymannx’s picture

And since the backlog of applications has been cleared up, it's maybe time to consider reinstalling the no-duplication requirement.

The different approach OP chose was to max aging cache to 0. Which should be considered harmful.

Once again, OP has never contacted me by any means although the duplication issue was mentioned often enough.

gisle’s picture

We agreed to repurpose the project application queue to be a queue for vetting users for the ability to opt their projects in to receive security advisory coverage.

I agree that security is very important,

For that we only have to review their ability to write secure code.

I think that is a shame. We really also should review that applicants are familiar with community standards beyond security. However, if there is no interest in doing this, I guess there is little I can do to get it back.

mmjvb’s picture

Would expect:
- Current tasks to be processed according current policy
- Separate issues for desired changes to current policy
- Webmasters to follow and enforce current policy
- DA governance to put an end to this inappropriate discussion, remember https://www.drupal.org/security-team/report-issue.

apaderno’s picture

Being able to write code that follows the coding standards is still something these applications consider, but not at the point to block the application until every single issue has been fixed.

I have approved these applications since Drupal.org used CVS. Since then, not duplicating the purpose of an existing project has not been considered a requirement, even if just cloning an existing project (i.e. creating a new project with git clone) to open an application would not make that application acceptable.
These applications are ran from volunteers. If nobody points out there is a too similar project before the application is approved, the status given with the application isn't revoked.

leymannx’s picture

If nobody points out there is a too similar project before the application is approved, the status given with the application isn't revoked.

And if it has been pointed out and the application has been approved anyways? What then?

leymannx’s picture

Is it really that difficult to acknowledge that how things went here are not ideal? Is it impossible to see why someone would feel offended by how OP handled the issue and describes someones others work the way they still do on the project page? Is there no way to find a balanced answer for all participants?

leymannx’s picture

Both you and Klausi got the notification when OP answered me I'd "destroy all contribute idea". No reaction from you. And now I still only hear: "Hey there's no problem". Cool? Not cool!

apaderno’s picture

I usually don't comment when it's not required a comment as webmaster or git administrator. A wrong opinion expressed from a user who didn't name-call another user doesn't require a webmaster's intervention.

As for less worrying about projects with similar purposes, that happened time ago, on Drupal.org, not on this issue queue or the equivalent used for CVS accounts. If it weren't so, a project like Group could not have been born, since it would have seen as duplicating the effort of an already existing project.

If these applications became security coverage applications is because the community felt the need to make them lighter, not because two users thought it was necessary. (There have been users committing code to implement the opt into security coverage feature in Drupal.org projects, and that didn't involve me nor, for what I know, the other user mentioned here.)

leymannx’s picture

Honestly I don't understand what exactly you are trying to say. It's OK to duplicate modules on drupal.org (spread the word). The project applications are merely for security issues (bad luck for me). When someone feels insulted you don't intervene as it's none of your business as a webmaster or Git administrator (you forgot your role as community member but OK). And you (or Klausi) didn't invent all this. This makes no sense to me. Sorry, I'm out. I'm quite disappointed actually.

apaderno’s picture

Feels insulted doesn't mean somebody has been insulted. It's not the task of a webmaster to intervene when somebody feels insulted, if not to remind everybody to keep a civil tone. Since it was not necessary in this case, I didn't comment at all.

And no, I haven't said that the purpose of these applications is merely to check for security issues, but that Drupal.org stopped worrying about modules with similar purposes time ago, which is different of talking of duplicate modules.

leymannx’s picture

Feels insulted doesn't mean somebody has been insulted.

Is this from drupal.org code of conduct?

I see my work presented in a disrespectful manner on https://www.drupal.org/project/user_status_online. OP started their module from an exact copy of my work. OP never raised an issue in the original module's issue queue. OP insulted me.

I acknowledge that you or Klausi don't want to or can't handle this. But is has to be handled somehow.

daffie’s picture

@leymannx: I am sorry to see that you are unhappy that you module has been copied. I am not saying that you should not be angry or hurt, but maybe you can look at it in a different way. More like the phrase: "Imitation is the sincerest form of flattery". They like your work so much that they have copied it. Maybe you can be proud of it.

klausi’s picture

@leymannx: the next step in our code of conduct and conflict resolution policy https://www.drupal.org/conflict-resolution is to schedule a meeting in a real time medium (best a video call) with the people involved, crafter in that case. Most of the time it helps to talk directly to get a problem resolved.

leymannx’s picture

@klausi – We already had direct contact. The outcome can be found some few comments above. I surely not gonna video chat with them after that. OP had quite some time to address the concerns not raised only by me. What happened?

leymannx’s picture

I reported this incident on https://www.drupal.org/governance/community-working-group/incident-report. I hope this can be resolved soon.

leymannx’s picture

I created feature request #3115715: Add "Ensure your module is not a duplicate" back to security advisory coverage application checklist to add the no-duplication requirement back to the application checklist and the review process. Please support me if you'd like to prevent incidents like this in the future. Many thanks

apaderno’s picture

gdemet’s picture

I wanted to chime in on behalf of the Drupal Community Working Group, who reviewed this thread after it was reported to us.

As our Code of Conduct states:

The Drupal community and its members treat one another with respect. Everyone can make a valuable contribution to Drupal. We may not always agree, but disagreement is no excuse for poor behavior and poor manners. We might all experience some frustration now and then, but we cannot allow that frustration to turn into a personal attack. It's important to remember that a community where people feel uncomfortable or threatened is not a productive one. We expect members of the Drupal community to be respectful when dealing with other contributors as well as with people outside the Drupal project and with users of Drupal.

We understand that leymannx was frustrated with how this issue was handled; however we felt that some of the language they used (e.g., "Honestly I wish your status would be revoked for the course of action you set here." in #34) unnecessarily escalated the situation, particularly given that up until that point crafter had been very accommodating of every request made of him in the thread.

We do agree that the language used on the user_status_online project page to contrast the two modules was unclear and could be read in a negative way, though we do not think the maintainer intended it that way. We've filed an issue to improve this language.

We do not believe that anyone intended to insult anyone else in this thread. However, we want to be clear that it is possible for people to inadvertently insult others without intending to do, and when someone says they feel insulted, it's important to take that seriously. It is the shared responsibility of everyone in our community to make sure everyone is treated with dignity and respect. As our values and principles state:

The expectation of the entire Drupal community is to be present and to promote dignity and respect for all community members. People in our community should take responsibility for their words and actions and the impact they have on others.

At this point, we are considering this issue (and this matter) closed.