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

Fix documentation in lib/views/main_screen.dart #1470

Closed
palisadoes opened this issue Feb 7, 2023 · 21 comments · Fixed by #1482
Closed

Fix documentation in lib/views/main_screen.dart #1470

palisadoes opened this issue Feb 7, 2023 · 21 comments · Fixed by #1482
Assignees
Labels
bug Something isn't working documentation Update documentation feature request

Comments

@palisadoes
Copy link
Contributor

Describe the bug

  1. Our push workflow Add tests for UserTasksPage (#1468) is failing.
  2. This is most likely due to recent changes to lib/views/main_screen.dart
  3. This has probably never been noticed before as we have not been adding new features to the app since we implemented the documentation workflow.

To Reproduce
Steps to reproduce the behavior:

  1. Push code to repo

Expected behavior

  1. No error
  2. A way to detect these types of errors as part of the PR process. Possibly adapting the documentationcheck.py script

Actual behavior
Error:

Run git branch
* develop
Defaulting to user installation because normal site-packages is not writeable
Collecting GitPython
  Downloading GitPython-3.1.30-py3-none-any.whl (184 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 184.0/184.0 KB 4.9 MB/s eta 0:00:00
Collecting gitdb<5,>=4.0.1
  Downloading gitdb-4.0.10-py3-none-any.whl (62 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 62.7/62.7 KB 11.8 MB/s eta 0:00:00
Collecting smmap<6,>=3.0.1
  Downloading smmap-5.0.0-py3-none-any.whl (24 kB)
Installing collected packages: smmap, gitdb, GitPython
Successfully installed GitPython-3.1.30 gitdb-4.0.10 smmap-5.0.0
🔍 DOCUMENTATION NOT UPDATED: Files with missing or not updated documentation found
>>> File name: lib/views/main_screen.dart
	DocumentationStatus.not_updated

Error: Process completed with exit code 1.

Screenshots
If applicable, add screenshots to help explain your problem.
image

Additional details
Add any other context or screenshots about the feature request here.

Potential internship candidates
Please read this if you are planning to apply for a Palisadoes Foundation internship #359

@palisadoes palisadoes added the bug Something isn't working label Feb 7, 2023
@github-actions github-actions bot added documentation Update documentation feature request unapproved Unapproved, needs to be triaged labels Feb 7, 2023
@literalEval
Copy link
Member

literalEval commented Feb 11, 2023

@palisadoes recently while writing tests for main_screen.dart, I refactored some parts of it and the subsequent PR didn't show this error. Can you please explain what does Missing or not updated documentation means ? Because I have written tests for quite a lot of files and never got this error even though those files didn't have any documentation written for them.

@palisadoes
Copy link
Contributor Author

  • It means that the file edited has no comments that can be extracted by DartDoc.
  • Please comment your code's functions, classes and methods so that others will find it easier to follow

@literalEval
Copy link
Member

Understood. Can I take up this issue and edit the documentation of this file ?

@palisadoes palisadoes assigned literalEval and unassigned noman2002 Feb 11, 2023
@palisadoes palisadoes removed the unapproved Unapproved, needs to be triaged label Feb 11, 2023
@literalEval
Copy link
Member

literalEval commented Feb 11, 2023

image

@palisadoes according to this documentation, the pluginList is never supposed to be null in production and the fallback plugin lists are just for testing purposes. Is this the expected behavior ? If yes, then I will simply move all of this fallback data to mock stubs and make the widget clutter free.

@palisadoes
Copy link
Contributor Author

Is this related to #1469 (comment)?

  1. We should be mocking as a best practice, that probably would have reduced that risk.
  2. If related, also restore any tests that were removed

@literalEval
Copy link
Member

Sure. Also, I wanted to ask, what exactly is this plugin thing ? Like these plugins are mapped onto BottomNavigationBarItem, so what does that mean for the user ?
Also, from the logic of the below image, only the 'pluginNames' that are hard coded will be added to bottom navigation bar. How does that make the 'plugin system' dynamic if only the selected items can be shown at any time ?

image

@palisadoes
Copy link
Contributor Author

@noman2002 @xoldyckk @CyberWake @rutvik11062000 I can't answer the specifics of this question. Please respond if you have an answer.

@literalEval
Copy link
Member

@noman2002 @CyberWake @rutvik11062000 from what I understood till now, the plugin system is the amount of control the user has over what they will be shown. Like a basic user will only see OrganizationFeed, ProfilePage etc while some others can see other pages as well depending on which plugins their orgs are using.

So currently we are storing an exhaustive list of plugins that we support and depending on whether it is 'installed' for the user, we show it or vice versa. Am I correct ?

@CyberWake
Copy link
Contributor

@SiddheshKukade since from this I am not sure you added this to the codebase. @palisadoes I currently have some insufficient input on this. Still looking into this as first glance I think it has all to do with the server and admin side configuration. Like if admin has not configured chat feature then users for that entity won't be able to see the chat navigation icon hence limiting the app with other feature.

image

@palisadoes according to this documentation, the pluginList is never supposed to be null in production and the fallback plugin lists are just for testing purposes. Is this the expected behavior ? If yes, then I will simply move all of this fallback data to mock stubs and make the widget clutter free.

@rutvik11062000
Copy link
Contributor

Hi @literalEval,
Regarding the plugins, we needed to make sure that through admin page, admin can enable / disable certain functionality (eg. chat) for their group, so we have introduced this plugin based thing, which fetches from the server through FetchPluginList(); then save it to hivebox.

Ideally for production, pluginList fetched from server should never be empty, but in case for the developers to work on all the functionality, we might have done this, where developer can locally send "null" as plugin response (or can directly do pluginList = { instead of pluginList ??= { )and auto set all the available plugins.

Please refer this and raise questions if any thanks.

@literalEval
Copy link
Member

literalEval commented Feb 14, 2023

Hello @rutvik11062000 , thanks for taking your time to reply. I have some questions.

  1. What exactly are the 'core' features that will always be available no matter what ? Are they 'OrgFeed' and 'ProfilePage', or some others are too ?
  2. Why do we need to continuously check for plugins ? Like if something is enabled or disabled for a user, we will get that data on the first fetch and show pages accordingly. Why do we need to re-fetch this data on each frame ?
  3. Why is the plugin fetching function implemented like so ? i.e., an object (FetchPluginList) is instantiated just for the sake of calling a function. Why are we not just calling that function directly ? Is there any design principle behind this which I don't know about ?

Please elaborate when you get time. Since the MainScreen is the first thing a user sees when actually using the app, I want it to be the best of what we can offer.

@SiddheshKukade
Copy link
Member

image

@palisadoes according to this documentation, the pluginList is never supposed to be null in production and the fallback plugin lists are just for testing purposes. Is this the expected behavior ? If yes, then I will simply move all of this fallback data to mock stubs and make the widget clutter free.

@literalEval @CyberWake @palisadoes Hi, I'm am a little late to take a look at this now. But those comments and dummy data we're added to make sure that basic plugins work without any plugin store in the database.
The detailed comments and data were supposed to be there for future references. That data should not have been deleted in #1482

Error Described in issue:

Run git branch
* develop
Defaulting to user installation because normal site-packages is not writeable
Collecting GitPython
  Downloading GitPython-3.1.30-py3-none-any.whl (184 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 184.0/184.0 KB 4.9 MB/s eta 0:00:00
Collecting gitdb<5,>=4.0.1
  Downloading gitdb-4.0.10-py3-none-any.whl (62 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 62.7/62.7 KB 11.8 MB/s eta 0:00:00
Collecting smmap<6,>=3.0.1
  Downloading smmap-5.0.0-py3-none-any.whl (24 kB)
Installing collected packages: smmap, gitdb, GitPython
Successfully installed GitPython-3.1.30 gitdb-4.0.10 smmap-5.0.0
🔍 DOCUMENTATION NOT UPDATED: Files with missing or not updated documentation found
>>> File name: lib/views/main_screen.dart
	DocumentationStatus.not_updated

Error: Process completed with exit code 1.

I don't thik this error is caused due to dummy data present in the document and also due to the comments.

@literalEval Your changes might need to need to be reverted because they're breaking the entire plugin architecture which was created by me.

@SiddheshKukade
Copy link
Member

I've also added the CAUTION: flag in the comments means these were not supposed to be removed or deleted to make the plugin's work.

@literalEval
Copy link
Member

@SiddheshKukade Thanks for taking time to reply. Can you please clarify how does #1482 break the plugin architecture ? The only thing that has changed is that Chat, Event and Post pages are now default, that too I took from #1465 as that PR suggested that these should be the default ones. I tried asking about what should be the default ones and what shouldn't be, and no one responded, so I thought of going with #1465.

So if you can clarify which pages should be default and which should be available as plugins, I will remove corresponding pages from being hard-coded to being dynamically updated. The plugin system is not broken, just some things are made default.

@SiddheshKukade
Copy link
Member

SiddheshKukade commented Feb 15, 2023

@literalEval Chat, events and post was already working as a default due to that dummy data. You deleted that data and now they're not connected with the plugin store in the admin and staying static which is not a intended behavior of plugins.
I basically mean Chat, event and post are now uncontrollable on admin due to your deletions

@literalEval
Copy link
Member

literalEval commented Feb 15, 2023

@SiddheshKukade so I will create a new PR in which I will make them non-default. There is no need to revert anything.

@literalEval
Copy link
Member

Hello @rutvik11062000 , thanks for taking your time to reply. I have some questions.

  1. What exactly are the 'core' features that will always be available no matter what ? Are they 'OrgFeed' and 'ProfilePage', or some others are too ?
  2. Why do we need to continuously check for plugins ? Like if something is enabled or disabled for a user, we will get that data on the first fetch and show pages accordingly. Why do we need to re-fetch this data on each frame ?
  3. Why is the plugin fetching function implemented like so ? i.e., an object (FetchPluginList) is instantiated just for the sake of calling a function. Why are we not just calling that function directly ? Is there any design principle behind this which I don't know about ?

Please elaborate when you get time. Since the MainScreen is the first thing a user sees when actually using the app, I want it to be the best of what we can offer.

@SiddheshKukade can you also please answer points 2 and 3 if you have some time ?

@SiddheshKukade
Copy link
Member

You can found about that in the docs.

@literalEval
Copy link
Member

I am really sorry @SiddheshKukade but I couldn't find anything about points 2 and 3. Especially point 3. Can you please elaborate it to me ?

@SiddheshKukade
Copy link
Member

Details about those points
2. This is feature of the plugins to make them work in real time.
3. we've to process those plugins according to where they're located in the user interface.

@literalEval
Copy link
Member

@SiddheshKukade

  1. I understand that, but my question is why do we need this to be real time ? Say an admin has installed the Donation plugin for their organization, then why would they need to uninstall and reinstall it frequently ? Can you please elaborate this point with an example ?
  2. I'm not asking about the processing part. What I am trying to understand is that why is this implemented like so ? Look here

image

A FetchPluginList object is instantiated. But all that this object does is that it calls a function in its constructor

image

My question is why is fetchList not implemented as a static method ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Update documentation feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants