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

Created equivalent of getters for all accessed session state and all getters in useUser composable #11738

Closed

Conversation

dhruvpal05
Copy link

@dhruvpal05 dhruvpal05 commented Jan 17, 2024

Summary

I modified the getter names in useuser.js to include the 'session/' namespace because these getters are defined in the session.js module. Vuex uses namespaces to organize state, getters, mutations, and actions from different modules.
I added the specified getters to the session.js module with the 'session/' namespace. These getters are the ones being accessed in the useUser composable. Additionally, I adjusted the existing getters to use the 'session/' namespace where necessary.

These changes ensure that the getters from session.js are properly accessed through the useUser composable in useuser.js.

References

Closes #11723

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

const isAdmin = computed(() => store.getters.isAdmin);
const isSuperuser = computed(() => store.getters.isSuperuser);
const canManageContent = computed(() => store.getters.canManageContent);
const isLearnerOnlyImport = computed(() => store.getters['session/isLearnerOnlyImport']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what was intended at all.

The purpose of the issue is to continue the work that was started here, providing composable interface to each of the getters in the session module, and to each element of the store state.

No changes to these existing exports should be made, and no changes should be made to session.js.

Instead, I should be seeing additional computed props returned from this composable that map to each of the getters defined in session.js and to each of the state values defined in the same file.

@akolson
Copy link
Member

akolson commented Jan 18, 2024

Hi @dhruvpal05! could you also add the actual GH issue you are working against so there is better context for the reviewer. Under References section you can use any of Closes #<GH Issue>, Fixes, Completes<GH Issue> so the issue can be auto closed on pr merge.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dhruvpal05! Based on @rtibbles review, this will certainly require changes. Please be sure to let us know incase you have a clarifying questions answered. Thanks

@dhruvpal05
Copy link
Author

Hi @akolson , thank you for the review, I want to ask that I have to make getters for the functions whose getters are not defined in session.js like this 'session/isAppContext': (state) => state.app_context, and make corresponding composable in useUser.js like this const isAppContext = computed(() => store.getters['session/isAppContext']); , correct me if I am wrong anywhere.

@rtibbles
Copy link
Member

Hi @dhruvpal05, no need to define any additional getters in the vuex session.js module here.

As a guide, if you have edited session.js while doing this issue, you have made a mistake.

The new computed props in the composable can simply directly reference the existing state or getters from the module.

@dhruvpal05 dhruvpal05 reopened this Jan 22, 2024
@dhruvpal05
Copy link
Author

@akolson @rtibbles made some changes ,if it is still not perfect please let me know.

@akolson
Copy link
Member

akolson commented Jan 22, 2024

Hi @dhruvpal05! Thanks. It, however looks like there are conflicts that you will need to fix before a proper review can be done.

Copy link
Contributor

github-actions bot commented Jan 23, 2024

Build Artifacts

@rtibbles
Copy link
Member

As @akolson mentioned, you have some unresolved merge conflicts that you have committed.

In addition, it seems like this guidance is still not being followed here:

As a guide, if you have edited session.js while doing this issue, you have made a mistake.

The new computed props in the composable can simply directly reference the existing state or getters from the module.

@MisRob
Copy link
Member

MisRob commented Feb 15, 2024

Hello @dhruvpal05, are you planning to follow-up here or would it be better to close the pull request?

@dhruvpal05 dhruvpal05 closed this Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create equivalent of getters for all accessed session state and all getters in useUser composable
4 participants