feat:renamed currentFacilityId

[Ghat0tkach]

Summary

This PR fixes #10763
Renamed the currentFacilityId to userFacilityId as instructed all with the help of @thanksameeelian

References

…

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

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a14.dev0_git.175.ge119146d.pex
Windows Installer (EXE)	kolibri-0.16.0a14.dev0+git.175.ge119146d-unsigned.exe
Debian Package	kolibri_0.16.0a14.dev0+git.175.ge119146d-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a14.dev0+git.175.ge119146d-0.3.0.dmg
Source Tarball	kolibri-0.16.0a14.dev0+git.175.ge119146d.tar.gz
WHL file	kolibri-0.16.0a14.dev0+git.175.ge119146d-py2.py3-none-any.whl

[thanksameeelian]

this is looking really good @Ghat0tkach!

there is one additional file where currentFacililtyId is still being used, in our old friend kolibri/plugins/coach/assets/src/modules/pluginModule.js, here on line 67.

this was the file that had previously been the source of the merge conflict, as its underlying code had changed, so resolving that conflict was necessary. however, i accidentally misinformed you when i said that file needed no updates - it does still reference currentFacilityId, just in a slightly different place than where it had previously (leading to the conflict we saw). if you can update that reference to currentFacilityId there in its new location, i think you'll have fully covered everything 🙂 i'm sorry for being confusing in my previous comment!

[Ghat0tkach]

this is looking really good @Ghat0tkach!

there is one additional file where currentFacililtyId is still being used, in our old friend kolibri/plugins/coach/assets/src/modules/pluginModule.js, here on line 67.

this was the file that had previously been the source of the merge conflict, as its underlying code had changed, so resolving that conflict was necessary. however, i accidentally misinformed you when i said that file needed no updates - it does still reference currentFacilityId, just in a slightly different place than where it had previously (leading to the conflict we saw). if you can update that reference to currentFacilityId there in its new location, i think you'll have fully covered everything 🙂 i'm sorry for being confusing in my previous comment!

thanks for pointing it out , actually my current branch is still behind the develop-branch, could you please suggest me a way to fetch the current branch without removing my commits since the pluginmodule.js is outdated in mine

[thanksameeelian]

thanks for pointing it out , actually my current branch is still behind the develop-branch, could you please suggest me a way to fetch the current branch without removing my commits since the pluginmodule.js is outdated in mine

sure thing, @Ghat0tkach!

have you followed the steps laid out here when setting things up? once you follow these steps and have develop set up as a remote called upstream, you can fetch changes that have been made to develop and rebase to get your branch back up to speed. you only have to do the setup part once, but will likely frequently need to fetch & rebase to keep up-to-date.

there are a few different ways to do this, but i do the following from my PR branch:
git fetch upstream develop
then
git rebase upstream/develop

these two commands should work to keep your branch up-to-date. it may be that during the rebase, you are alerted to merge conflicts. you can resolve those in your code editor or with your preferred method, while being careful while reconciling the two that you are only introducing the changes you intend 🙂

[Ghat0tkach]

this is looking really good @Ghat0tkach!

there is one additional file where currentFacililtyId is still being used, in our old friend kolibri/plugins/coach/assets/src/modules/pluginModule.js, here on line 67.

this was the file that had previously been the source of the merge conflict, as its underlying code had changed, so resolving that conflict was necessary. however, i accidentally misinformed you when i said that file needed no updates - it does still reference currentFacilityId, just in a slightly different place than where it had previously (leading to the conflict we saw). if you can update that reference to currentFacilityId there in its new location, i think you'll have fully covered everything 🙂 i'm sorry for being confusing in my previous comment!

I am afraid I didnt added the git remote add upstream [email protected]:learningequality/kolibri.git this time , Can i add it now and do the suggested steps? I really apologise 😭

[thanksameeelian]

no worries, @Ghat0tkach! adding the upstream remote should not have any negative impact on the work you've done.

here are the ordered steps you can do to get things working:

1. do you currently have local branch called develop? if so:
   a. if there's work on that branch that you'd like to preserve, make a new branch off of it that retains these changes (git checkout -b origindevelop)
   b. delete your local develop branch (git branch -D develop)
2. follow the steps provided in the final code block in "Checking out the code" of the developer documentation to set up upstream
3. after performing the final command from this documentation (git checkout -t upstream/develop), check out your feature branch (git checkout current_Facility_Id)
4. then you can git merge develop (or git rebase develop) to get your branch up-to-date with the latest changes in develop
5. then push your code up as you have been in order for it to be accessible here. if your push is rejected and you get a warning similar to Updates were rejected because the tip of your current branch is behind its remote counterpart, you can force the push by adding --force to the end of your push command

[Ghat0tkach]

no worries, @Ghat0tkach! adding the upstream remote should not have any negative impact on the work you've done.

here are the ordered steps you can do to get things working:

1. do you currently have local branch called develop? if so:
   a. if there's work on that branch that you'd like to preserve, make a new branch off of it that retains these changes (git checkout -b origindevelop)
   b. delete your local develop branch (git branch -D develop)
2. follow the steps provided in the final code block in "Checking out the code" of the developer documentation to set up upstream
3. after performing the final command from this documentation (git checkout -t upstream/develop), check out your feature branch (git checkout current_Facility_Id)
4. then you can git merge develop (or git rebase develop) to get your branch up-to-date with the latest changes in develop
5. then push your code up as you have been in order for it to be accessible here. if your push is rejected and you get a warning similar to Updates were rejected because the tip of your current branch is behind its remote counterpart, you can force the push by adding --force to the end of your push command

I am really at loss of words for your immense help . Thank you

Edit: since i was dealing with some issues , and I had to commit only in the pluginModule.js , I thought why dont I copy the contents of pluginModule.js of the updated file into my old current-facility-id. This way my pluginModule.js will be already upto date with the current repo one.

In short - I fetched only the pluginModule.js from the current repo and did the required changes
.😅

Edit 2: Merge Conflicts again , working to resolve them
Edit 3; Fixed them finally phew

[thanksameeelian]

these changes are looking good to me, @Ghat0tkach! thank you for working so hard to keep everything up-to-date as the codebase changes underneath you - that's not an easy feat!

it looks to me like you've changed every instance of currentFacilityId to userFacilityId (including in comments & tests which is very helpful). so this looks good from a code perspective!

it appears like this is failing linting though, currently, which should be a straightforward fix - when you're in your branch, in your terminal you can run yarn run lint-frontend:format in order to get your code in line with the linting rules, and then commit and push those changes, which are likely quite small.

[radinamatic]

QA team can certainly take a look at this, could you add some review guidance steps @thanksameeelian ?

[Ghat0tkach]

Thanks a lot @thanksameeelian , it couldnt be possible without your guidance. @radinamatic actually one of the checks is failing and it could be due to empty line , here. Could you please take a look?

[thanksameeelian]

QA team can certainly take a look at this, could you add some review guidance steps @thanksameeelian ?

for sure @radinamatic! my apologies because i tagged you and then quickly edited my response to remove it because i realized there was a still a little linting work to be done before this is officially "ready." so i will re-tag you once that's settled!

while the effects of this PR could be a bit wide-ranging, i believe that places where its impact are most evident include coach & facility plugins - for a single-facility user but especially for a multi-facility user. related to recent work, i think it would be very helpful to check that for a multi-facility user, when navigating within those two plugins and between different classes/facilities, the page links are taking you to the place you intended to go (e.g. if you click a "← All classes" you are navigated to the correct class list from the facility you're currently in, and similar). it would also be helpful to double check that when you sign in to a specific facility, you're definitely within that facility, and within the user profile your default facility name is accurate.

i think those should mostly cover the situations touched by this, but i'll tag you again once this is truly ready for your perusal. thank you!

[thanksameeelian]

Thanks a lot @thanksameeelian , it couldnt be possible without your guidance. @radinamatic actually one of the checks is failing and it could be due to empty line , here. Could you please take a look?

@Ghat0tkach this is something that you & i can work out together, but our lovely QA team focuses on the functionality & accessibility of our code.

did you run yarn run lint-frontend:format locally? this will auto-format your code, correcting the linting problems, and all you have to do is commit and push those changes. it's usually more helpful than trying to parse the problems in the github action. it is evident from what you linked, however, that a problem with that file is the indentation, which should match the suggestion in the green line and the indentation that was present in the code before you changed it - with the const name on one line and the long definition of it on the next line.

[Ghat0tkach]

Thanks a lot @thanksameeelian , it couldnt be possible without your guidance. @radinamatic actually one of the checks is failing and it could be due to empty line , here. Could you please take a look?

@Ghat0tkach this is something that you & i can work out together, but our lovely QA team focuses on the functionality & accessibility of our code.

did you run yarn run lint-frontend:format locally? this will auto-format your code, correcting the linting problems, and all you have to do is commit and push those changes. it's usually more helpful than trying to parse the problems in the github action. it is evident from what you linked, however, that a problem with that file is the indentation, which should match the suggestion in the green line and the indentation that was present in the code before you changed it - with the const name on one line and the long definition of it on the next line.

Thanks a million :3 , fixed it lets hope for the test to pass

[thanksameeelian]

hey @Ghat0tkach, what the linter wants is for you to remove a small extra space on line 66:

on the left is what the linter has observed in your code, while on the right is what it would like to see. it doesn't require that most recent extra line you've included, but if you keep it the linter may also be alerting to extra spaces present in that line that are extraneous.

i learned this from running yarn run lint-frontend:format in my terminal and then observing the automated change in my code editor

if this isn't working for you, let me know and we can change tactics.

[Ghat0tkach]

hey @Ghat0tkach, what the linter wants is for you to remove a small extra space on line 66: on the left is what the linter has observed in your code, while on the right is what it would like to see. it doesn't require that most recent extra line you've included, but if you keep it the linter may also be alerting to extra spaces present in that line that are extraneous.

i learned this from running yarn run lint-frontend:format in my terminal and then observing the automated change in my code editor

if this isn't working for you, let me know and we can change tactics.

Thank you , and here i was doing trial and error method to let it work 😭😂

[thanksameeelian]

Thank you , and here i was doing trial and error method to let it work 😭😂

that's extremely relatable @Ghat0tkach - i've definitely done the same 😀

but look at this, linting has passed!! thank you so much for your persistence! 🚀🚀

[thanksameeelian]

@radinamatic - i believe this is ready for a small QA review now! if the guidance i provided here doesn't feel sufficient, please let me know and i can try to elaborate further!

[Ghat0tkach]

Thank you , and here i was doing trial and error method to let it work 😭😂

that's extremely relatable @Ghat0tkach - i've definitely done the same 😀

but look at this, linting has passed!! thank you so much for your persistence! 🚀🚀

All credit goes to you for being so helpful and supportive and to answer my silly doubts and forgive my mistakes😅🥳

[pcenov]

LGTM @radinamatic - no issues observed while regression testing.

[thanksameeelian]

this has passed code review & QA testing, so should be merged shortly. thanks for your contribution @Ghat0tkach!!

[Ghat0tkach]

this has passed code review & QA testing, so should be merged shortly. thanks for your contribution @Ghat0tkach!!

Thank you @thanksameeelian
Do I have to add my name in authors.md too?

[thanksameeelian]

this has passed code review & QA testing, so should be merged shortly. thanks for your contribution @Ghat0tkach!!

Thank you @thanksameeelian Do I have to add my name in authors.md too?

@Ghat0tkach, i've added your name & username on github to our AUTHORS.md file! please let me know if any changes need to be made for accuracy 🙂

[Ghat0tkach]

this has passed code review & QA testing, so should be merged shortly. thanks for your contribution @Ghat0tkach!!

Thank you @thanksameeelian Do I have to add my name in authors.md too?

@Ghat0tkach, i've added your name & username on github to our AUTHORS.md file! please let me know if any changes need to be made for accuracy 🙂

Thanks a lot !